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

Move build to rollup, create es module build, add sourcemaps #171

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

bennypowers
Copy link

Closes #159

@timrosskamp
Copy link

Nicely done! But there are still a few issues:

In the package.json "jsnext" needs to be replaced by "jsnext:main".

Eslint isn't working anymore. Maybe implementing the rollup-plugin-eslint plugin will fix this. But I haven't used eslint personally, so I can't say much about it.

In the rollup wiki it's recommended to still transpile es6 to es5 even when using the module field and only use the module syntax. So I would suggest to add babel to the plugin list of the module version.

If you're using things like arrow functions and classes (or more exotic features like decorators and object spread), you should transpile them if you want your library to work in environments that don't support those features. Otherwise, whoever uses your library will have to transpile it themselves, including configuring their transpiler to handle your code, which might involve esoteric transforms and conflicting versions of Babel.

It's a frequent source of frustration. Be a responsible library author and ship code that actually runs in the environments you support!

@timrosskamp
Copy link

I made a pull request so it's easier for you.

Configured babel, fixed package.json, fixed es module build
@bennypowers
Copy link
Author

@timrosskamp so I tried it out, and pre-transpiling to umd actually broke my app's bundle. I merged in your stuff, but I still feel strongly that for such a simple library, distributing straight up es6 is the way to go

@bennypowers
Copy link
Author

I added another build so authors can have choices:

  • siema.nomodule.js - bundled and minified just like back in the day
  • siema.es5.js - es module transpiled to es5
  • siema.es.js - regular old javascript that runs on all major browsers

@timrosskamp
Copy link

timrosskamp commented May 9, 2018

Don't you think thats this gets a bit confusing for the regular user? I've never seen a .nomodule.js bundle in any js libary before, since .min.js is the common naming convention. Many people still copy&paste code from the dist folder and normally search a [libaryname].min.js file to include in their website.

Siema is a simple slider, that should work in all major browsers including IE. Forcing every rollup or webpack user to implement and configure a transpiler like babel, because the module bundlers will use the es6 version of the code by default, is in my honest opinion a very bad practice. You either have to manually dive into the package and tell your module bundler to use the pre-transpiled es5 code bundle or you are forced to transpile the code yourself. That just doesn't sound right to me.

If I include this slider in my project, I would expect that I just have to import Siema from 'siema' and everything will work as expected, since IE10+ support is advertised in the README.md.

The module field in the package.json is not yet standartized, but across the npm-community it's definition is that's it points to a code build with es-modules, but no modern javascript like classes, arrow-functions and so on.

Quote from the rollup github wiki:

pkg.module will point to a module that has ES2015 module syntax but otherwise only syntax features that the target environments support.

@pawelgrzybek
Copy link
Owner

Hi all.

Thanks for a cool discussion here. My take:

I agree with @timrosskamp. Siema always have been, still is and will be a simple solution for not experienced users. Tons of users copy minified version of a lib, smash a link in a body and add a few lines of a configuration (exactly like it is presented on promo video).

Because it is lightweight and simple solution, more experienced users like it too . Thanks to UMD, we can download it from npm and use via module bundler with ease.

I am not a rollup user and I didn't realise that it requires some specific syntax / definition for a file. I will add it of course but in the least destructive way for a current implementation, because it is working well for community.

I will find a solution but first I need to find a little bit of time :)

Thanks for valuable discussion here guys. One more thing! Next version of Siema (2.0) drops IE 10 (this will reduce the package size by half), goes TypeScript and improves accessibility. Any comments on that? Any additional requests?

Thanks and have a nice day y'all 👋

@bennypowers
Copy link
Author

bennypowers commented May 10, 2018

I don't have a problem changing nomodule to min

I think perhaps @pawelgrzybek you maybe misunderstand my intentions. I DON'T mean that you should require users to use rollup. I DO mean that you should give users many options.

The point is to make the lib work for:

  1. users who just want to link to the minified code from a cdn (umd)
  2. users who want to use unpkg with <script type="module"> (es6 es, not transpiled - all module browsers support arrow funcs etc)
  3. users who want to npm i siema and import Siema from 'siema (es6, not minified, optionally transpiled)
  4. users who want to npm i siema and script type module (es6) or script (.min)

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.

3 participants