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

Author ES modules, use Rollup for bundling #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fpapado
Copy link

@fpapado fpapado commented Feb 14, 2018

Greetings, stranger!

This PR moves the source to ES6 modules, and uses Rollup (via https://github.com/developit/microbundle) to output CommonJS, UMD, and ES modules; it takes the options directly from the
relevant package.json fields, which is nice.

ES6 modules are nice because they can be statically analysed by bundlers. The library is tiny, so tree shaking / dead code elimination won't do much (plus the functions link between each other). Still, ES6 modules can be hoisted to the top scope by bundlers, and are easier to integrate (especially true for Rollup).

This turned out both simple and a tiny bit annoying, here's notes:

Exports
Using both default exports and named exports is allowed by the
spec, but not recommended. If we have both, then the usage
with require() needs to access .default, which is breaking, and
also can mess with bundling/scope hoisting. I moved the exports to
named only, and the tests run the same as before.

This is the important thing here, and I am not sure if it breaks
something else. I am curious to test it with typescript synthetic
default imports (import * as cssNs from 'css-ns'), which until 2.7
have odd behaviour. If it does break, then I defer to you :)

Tests
Tests now use the files in dist/, since they are CommonJS and node
does not yet understand ES modules.

NPM -_-
One annoying thing atm is that npm install adds many versions of
acorn, which leads to errors with microbundle. Using yarn fixes
that. There are no dependencies, so this shouldn't change anything
but it is something to consider.

Publishing helpers
In addition to the modules field, I added scripts for publishing
to npm and bumping the tag, plus the "files" option. It should
match the instructions at the end of the README, but we can omit
it / split to another PR / anything.

Using both default exports and named exports is allowed by the
spec, but not recommended. If we have both, then the usage
with require() needs to access .default, which is breaking, and
also can mess with bundling/scope hoisting. I moved the exports to
named only, and the tests run the same as before.

Rollup config is handled by microbundle, which takes the options
directly from package.json fields.

Tests now use the files in dist/, since they are CommonJS and node
does not yet understand ES modules.

One annoying thing atm is that npm install adds many versions of
acorn, which leads to errors with microbundle. Using yarn fixes
that. There are no dependencies, so this shouldn't change anything
but it is something to consider.

In addition to the modules field, I added scripts for publishing
to npm and bumping the tag, plus the "files" option. It should
match the instructions at the end of the README, but we can omit
it.
@fpapado
Copy link
Author

fpapado commented Feb 14, 2018

I am pretty sure the CI fails because it's trying to import the ES module from ./css-ns.js rather than dist/css-ns.js

@jareware
Copy link
Owner

Oooh, this looks great! Sorry for being laggy with looking at this, been really busy, but I'm getting back to it ASAP!

@jareware
Copy link
Owner

Sorry for taking forever with this!

This looks nice, definitely happy about modernizing the source a bit.

The Travis error is just due to the over-zealous engines spec in package.json. I'm fixing that on master as we speak.

Using both default exports and named exports is allowed by the spec, but not recommended.

I think it'd be fine to add an entrypoint file (say, index.js) which only has that single default export. If someone (e.g. the test runner) needs the other stuff, they can always import directly from css-ns.js.

Also, any idea what's up with this one:

$ npm install
...
$ npm run build

> [email protected] build /Users/jara/Projects/css-ns
> microbundle

Error: Plugin 'jsx' not found

...or do the yarn files mean that npm doesn't work anymore?

Finally, have you npm linked this and checked that it works on an existing project?

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

Successfully merging this pull request may close these issues.

2 participants