Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NPM module doesn't export #10

Open
gridsystem opened this issue Sep 19, 2024 · 6 comments
Open

NPM module doesn't export #10

gridsystem opened this issue Sep 19, 2024 · 6 comments
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@gridsystem
Copy link

gridsystem commented Sep 19, 2024

After installing p5.asciify from npm, it's not possible to import the module into an app, as it doesn't export anything.

I think it just loads itself into the global namespace?

Importing into a (very fast to set up) Vite.js app as an example,

import p5asciify      from 'p5.asciify';
// or
import { p5asciify }  from 'p5.asciify';

Results in

The requested module does not provide an export named 'default'
@humanbydefinition
Copy link
Owner

Hey @gridsystem!

Thanks for reaching out and making me aware of this. I have never tried those use-cases in particular, was hoping the recent release, including instance mode support, would handle all those cases.

I'll try to look into this more this weekend hopefully and will give an update. Thanks again, happy to improve on the library, so everybody is able to use it within his/her favorite environment.

@humanbydefinition
Copy link
Owner

Hey again @gridsystem, I looked into this now. Unfortunately that's out of reach for me currently.

I've set up a vite project and successfully ran a p5.js sketch, but adding p5.asciify doesn't work and causes issues I don't know how to solve yet.

I was able to generally import p5.asciify by exporting the p5asciify variable, which is located at the beginning of this libraries index.js file.

Within this index.js file, there are also some p5 hooks defined, which are necessary for the setup of the library. Those hooks do not work in this scenario for some reason, resulting in the add-on library to never set up and run.
Reference: https://github.com/processing/p5.js/blob/main/contributor_docs/creating_libraries.md#step-6

Also, functions that p5.asciify attaches to the p5 context, like calling sketch.setAsciiOptions(), do not seem to be present in this scenario, resulting in more errors.

Are there any other p5.js add-on libraries that work in those kinds of environments? It would be important that those add-on libraries also use p5 hooks or attach functions/methods to the p5 context to use as a reference.

My only idea would be to not rely on those hooks and require the user to do certain calls in preload(), setup() and at the end of the draw() function to make it work. I'd like to keep it simple for the user and rely on those hooks though, so I am hoping to find a solution at some point, if there is any currently.

@gridsystem
Copy link
Author

gridsystem commented Sep 20, 2024

Hey man. It’s late here so I’ll have a proper dig through this tomorrow. But I wanted to get a quick reply to you while it’s still fresh in your mind!

I imagine the reason P5.asciify isn’t playing nicely with P5 in that setup is that they are each in their own separate module namespace.

Usually when you’re publishing something to NPM, it would be packaged as a nodejs module, so that it can be installed with npm install my-thing. This isn’t by any means limited to Vite, it’s just the example I had to hand. I can see that you’re using import with the different files in the P5.asciify repo - it’s exactly the same.

If the module you’re installing has a dependency, so in your case P5, P5 would be marked as a dependency in package.json, so that when I npm install p5.asciify, if I haven’t already installed p5, npm would install that for me too.

This is really different to the <script src=“…” way of installing things. In that scenario, the scripts that I’m adding to my site would usually add things to the global namespace - var p5 = … creates a property on the window, window.p5. So if I then install p5.asciify, it can see p5 too, because they’re both just properties of window, in the global namespace.

I’m not teaching you to suck eggs here - just want to make sure we’re on the same page with terminology etc.

So the solution with p5.asciify might be as simple as having import p5 from ‘p5’ in your code, so that p5.asciify and p5 are in the same namespace?

What I’m not sure about though, is if you set that up in Rollup, how you would publish a script that can be imported via <script src=“… as well as import p5.asciify…. I experimented with making Rollup modules maybe 5 years ago so I’m not an expert but I have seen it done elsewhere, so I could do some research!

@gridsystem
Copy link
Author

gridsystem commented Sep 21, 2024

Hey @humanbydefinition !

I've had a play with the code today and found some examples from other p5 library authors.

My suggestion from last night was useless because your code then adds the methods to a different instance of p5, that I'd created inside your module.

I have worked out how to package the module for use in browser via script tags and for use as an import-able ES module.

I've also included a reference, which has done the same thing, and a few notes about how.

The reason I'm keen to get this working as an ES module is that so many sites and apps are built with Vue, React, or other frameworks that are built around modules, which don't make it easy to just add <script> tags or access the global namespace.

I'm not going to submit a full pull request, but here is what I've done with some comments:

// rollup.config.js
// Clearly differentiate between the minified file for browser `<script>` tags and an ES module for importing
…
// Rollup wants a name for an ES module
{ file: 'dist/p5.asciify.js', format: 'es', name: 'p5asciify' }, 
{ 
  file: 'dist/p5.asciify.min.js',
  // The module for browser script tage might use IIFE
  // https://rollupjs.org/tutorial/#using-output-plugins
  format: 'iife', 
  // Rollup needs a name for an IIFE
  name: 'p5asciify',
…
}
// tests/…/index.html
// Always use the minified version in browser `<script>` tags
<script src="../../dist/p5.asciify.min.js></script>

// package.json
// Always use the non-minified version in ES imports
"main": "dist/p5.asciify.js"
// src/index.js
// Export something for ES modules to import
export default p5asciify;

So with this setup, I can

  • test the minified file works via the tests in your /tests directory.
  • test the ES module can be imported by creating a Vite.js app, and running pnpm link ../p5.asciify to use the local version of p5.asciify instead of the published version on npm (https://pnpm.io/cli/link#use-cases)

But there would definitely be some changes to make so that it works when importing.

As you've said, in an ES module environment, p5.asciify doesn't have access to p5, and as I've tested, importing p5 at the top of your index.js doesn't give it access to the same version of p5 that you'd make your sketches with.

Here is a library, p5.brush, which works as an ES module and as a browser <script> tag. They're using Rollup to make a UMD which is different to separate ES module and minified versions for `<script> tags, and might be better than the way I've pieced together today.

In their instance mode example, they run brush.instance(p5Instance) to link the two modules together. The instance() method then runs _registerMethods(p5Instance), which then hooks into p5's afterSetup, post etc.

This isn't necessary with the <script> tag/minified way of doing things, as the UMD just exposes all of the exported methods inside index.js as properties of a brush object, so you can just run brush.scale() etc straight away from the global namespace and it already has access to p5 which is global too.

p5.brush is a great example, but to find others, I've found searching npm for p5 returns loads of libraries that would hopefully be published in the right format to work with npm and import or require().

So yes there is a little bit of work to do to make that happen, but it seems pretty achievable.

Let me know if that helps!

@humanbydefinition
Copy link
Owner

Hey again @gridsystem, sorry for coming back to you so late. I really appreciate the effort you put in, and I hope and am a little confident to soon provide a solution to the problem.

When I initially tested it and wasn't able to find a solution, I was a bit discouraged and worked on other things. Since I was made aware of another person with a similar report recently, I gave it another go today with better results. :)

I haven't done deeper testing yet, but once I did, I'm happy to release another update to finally fix this issue.

I've pushed a commit to the main-branch already, which I'll test more in the coming days: cd1baff

I'm bundling the library now also via umd format and essentially just added two statements to the index.js file.

The library files in the dist/ folder are also updated already and should work if pasted into the corresponding node_modules folder. But I don't expect you to test this for me obviously. I'll give an update in the coming days.

Cheers!

@humanbydefinition humanbydefinition added bug Something isn't working good first issue Good for newcomers labels Nov 13, 2024
@humanbydefinition
Copy link
Owner

Hey again @gridsystem, hope you are doing well!

I've just released a new update that hopefully fixes the issue with the library not working when imported via npm. I've tested it as you've described in a dummy Vite project and hope it'll work universally now when imported via npm.

https://github.com/humanbydefinition/p5.asciify/releases/tag/v0.3.0

I'd love to get feedback by another person in to confirm. Then, i'd be able to confidently close this issue for now. :)

@humanbydefinition humanbydefinition self-assigned this Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants