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

Need to test with bundlers #84

Closed
4 tasks
ericelliott opened this issue Sep 6, 2017 · 11 comments
Closed
4 tasks

Need to test with bundlers #84

ericelliott opened this issue Sep 6, 2017 · 11 comments

Comments

@ericelliott
Copy link
Collaborator

ericelliott commented Sep 6, 2017

Because the popular bundling tools may not bundle stuff from node_modules our package.json may need to point to bundled versions of the library, instead of individual modules. I've tried doing this with Browserify, but then Webpack gives weird warnings to all the users of our module (I think -- it used to, anyway).

We could try to accomplish this with rollup.

For this to be complete we need to test:

  • npm install from the npm tar bundle
  • Build and use in Webpack
  • Build and use with Browserify
  • Build and use with Rollup

cc @MarkHerhold @gmaclennan

@ericelliott ericelliott added the bug label Sep 6, 2017
@MarkHerhold
Copy link
Contributor

Just to clarify the point on node_modules, we currently don't have any node_modules to bundle as this library only has dev dependencies in package.json.

@ericelliott, could you give a specific use case of someone running into an issue using cuid? I'm guessing this case wouldn't involve a build tool like webpack or browserify since these tools already bundle everything that is needed.

@gmaclennan
Copy link
Contributor

I'm unclear about the exact nature of the bug? Both webpack and browserify should be fine with bundling with the main field pointing to index.js, although I have not tested webpack with the latest version. I am unclear about how Webpack respects the browser field in package.json - the docs state that it is supported but perhaps only as string values?

We could include the build files in the npm bundle by adding an empty .npmignore file (which would stop npm from ignoring the dist in the .gitignore - https://docs.npmjs.com/misc/developers#keeping-files-out-of-your-package) then users could require('cuid/dist/cuid'), however, I notice that we only build a node version, not a browser or react-native version. We should probably fix that.

Any feedback from users about what warnings or errors they are getting from Webpack would help resolve things.

@gmaclennan
Copy link
Contributor

Ok I see the problem. The files field in the current package.json means that the lib folder is not being included in the tarball when published to npm, so everything will fail when installing from npm. Probably best just to remove that field, and use .npmignore to blacklist rather than whitelist files. I'm just heading out for the weekend now, so probably won't get round to doing this until next week.

FYI I tested the bundle without publish with npm pack then from a tmp local folder npm i ../cuid/cuid-2.0.0.tgz

@ericelliott
Copy link
Collaborator Author

FYI I tested the bundle without publish with npm pack then from a tmp local folder npm i ../cuid/cuid-2.0.0.tgz

I hadn't thought of that. Thanks for the tip!

@ericelliott ericelliott changed the title Need to point to bundles Need to test with bundlers Sep 11, 2017
@ericelliott
Copy link
Collaborator Author

Any progress?

@pyrossh
Copy link

pyrossh commented Oct 22, 2017

We need to add fuse-box also as a bundler. Its not working since its trying to load the os package. Wouldn't it better if we have 2 packages.
Anyways fusebox provides a global process env variable with these fields
{title: "browser", browser: true, env: {…}}. Maybe we could use this to switch which packages are loaded.
And also what about react-native, how would it run in that environment?

@ericelliott
Copy link
Collaborator Author

Good questions. Please explore and let me know what you figure out. =)

@notrab
Copy link

notrab commented Apr 11, 2018

I'm having issues with using rollup and I was wondering if anyone here has had a similar issue?

The error I get back is Uncaught TypeError: n is not a function and the sourcemap reveals this to be a function that returns cuid().

If I replace cuid() with something else, it works.

Love to hear any suggestions @ericelliott 😄

@ericelliott
Copy link
Collaborator Author

@notrab

If the function returns the result of calling cuid(), that would be a string, not a function. We'd need more context to say much more. What does the function look like, and what is its intended purpose?

@nevf
Copy link

nevf commented Apr 22, 2018

@ericelliott Same problem using parcel.js - "Its not working since its trying to load the os package."

@ericelliott
Copy link
Collaborator Author

I think our new browser tests have this covered?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants