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

Add rollup config for UMD an ESM modules #7

Merged
merged 3 commits into from
Jul 6, 2018
Merged

Add rollup config for UMD an ESM modules #7

merged 3 commits into from
Jul 6, 2018

Conversation

madwizard-thomas
Copy link
Contributor

Here is my PR for issue #5. I have modified the main JS file to use ES6 exports and removed the wrapping function (note that the code should be indented to the left 1 tab but I kept the indentation to keep the PR cleaner). With a modern browser you could directly use this file as a JS module. However it is good practice to generate an distribution ES module file with exports but with all other features transpiled to a specific target. This is the dist/webauth-simple-app.esm.js file. It is ES5 compatible apart from the exports. This is done by the babel plugin (babel-preset-env as preset).
For CommonJS/AMD or <script> tag use a UMD module is built to dist/webauthn-simple-app.js.

You can use npm run build or npm run build:prod to build these distribution files (currently the only difference is the generation of source maps).

One problem I came across were the tests. On the node side, commonJS modules are expected so it no longer works with the new export statements. I have solved this by referring to the dist file instead. Downside is that you need to run build before test. If you want to keep using the actual source file in tests then alternative are using ES6 import statements in the tests as well or using mocha --require babel-core/register which will magically make it work (it will transpile the modules on the fly). However for the shared test code and the actual browser tests the former will only work with ES6 browsers and the latter can not work in a browser environment.

On the browser side I also modified the code to use the dist file instead. Another change that had to be made in the shared test code on the browser side is that all exports are now grouped in one single global object, window.WebAuthnSimpleApp (btw, this name is pretty long but you can change it to something else in the rollup config if you want). This is a breaking change but it is more similar to how commonJS works and prevents global namespace pollution.

@madwizard-thomas
Copy link
Contributor Author

Babel does add some boilerplate functions to the top of the distribution files, a much cleaner alternative is bublé but it doesn't translate as much features as babel. I tried it with your code, had to modify the 'for..of' since it is not supported and it worked but crashed on an infinite loop in stringifyArr, I think buble has some bug transpiling ...obj[key]. So I kept babel instead.

@apowers313 apowers313 self-requested a review June 28, 2018 05:22
@apowers313
Copy link
Collaborator

Thanks for this. I'm really excited to check it out, but I probably won't get time before this weekend.

@apowers313
Copy link
Collaborator

This is awesomw, thanks again for putting this together. Some quick thoughts:

  1. Can you commit the dist files?
  2. I think I’d rather test against dist, which makes more sense because it ensures that the build is functional. I also have (perhaps unreasonable) preferences against using babel
  3. Can you ‘npm run docs’ to make sure that still works?
  4. I think I will split up the single file into class-based files after we merge this PR

Let me know what you think.

@apowers313
Copy link
Collaborator

  1. Looks like it’s already testing against fist and Babel is no longer used?
  2. docs still work
  3. I started working on this... I’m curious what a good ES6 module design pattern is for splitting out Msg, ServerResponse, AttestationResult, etc into separate files. Do I need Rollup for ES6 at that point? Should I just have an index.js that aggregates the classes?

@madwizard-thomas
Copy link
Contributor Author

  1. Yes it is testing against dist already. Because the test code itself is ES5 it will work without babel. Babel is still being used by rollup though but once the dist file is created you are babel free :) I think Babel is the de facto standard transpiler, you can look into Buble if you want something leaner but it will require some minor code adjustments and you have to watch out for what features are supported. Babel can handle pretty much anything.
    Because of the module and main references in package.json, you can also use require("../../dist/") for commonJS and import ... from "../../dist" for ES6 and it will automatically pick the right one.
    Also note that while babel transpiles the syntax to valid ES5, the current configuration does not automatically add polyfills for ES6 classes such as Map or Set, so you might still get errors with internet explorer. Of course webauthn won't work in any case but you don't want javascript errors to show up when your library is loaded.
  2. I have wondered about that to, strictly speaking you don't need rollup if you use ES6 and you can the entrypoint directly. Most libraries I come across have a built dist file though. For browser use the code is often still transpiled to ES5 syntax apart from the export/import statements. The developer using your library could transpile the code themselves if needed but transpilers often have many options and it might break your code so sticking to ES5 is often done to prevent this. I can't think of many other reasons.

@apowers313 apowers313 changed the base branch from master to es6-module July 6, 2018 16:27
@apowers313 apowers313 merged commit 05bbc3a into webauthn-open-source:es6-module Jul 6, 2018
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