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 module and umd build configs for webpack/parcel/browserify/etc support #68

Closed
wants to merge 2 commits into from

Conversation

steve8708
Copy link

@steve8708 steve8708 changed the title Add module and umd configs for webpack/parcel/browserify/etc support Add module and umd build configs for webpack/parcel/browserify/etc support Feb 25, 2019
@steve8708
Copy link
Author

This should allow (nearly) anyone with any build tool to safely import SES from 'ses' and go without issues across tools or environments. Supports require('ses') for browsers and node as well.

It's based off the build I use for Builder pacakges that has been tested across customers across all sorts of build tools and environments, and is based off of other rollup best practice examples for npm modules like this and this

@steve8708
Copy link
Author

Also worth noting - typically it's suggested to not commit those dist files and to gitignore them instead and have your CI build them, but I figured it's worth just making one change here at a time

@warner
Copy link
Member

warner commented Feb 25, 2019

What's the best practice for dist files like that.. should we have them in the repo at all? We use them for the demo (demo/index.html has a script tag for one of them), but in general it's a PITA to have those checked in. We aren't quite ready to host bundled copies anywhere yet (like what jquery does). Having CI build them makes perfect sense but then what should we do with the generated files?

@steve8708
Copy link
Author

Ah yes, you don't need to host them anywhere. When you npm publish as long as dist isn't in your npmignore (it's not) then they will go up to NPM.

These days there are many services that host NPM modules for everyone without you having to do anything, such as unpkg and jsdelivr

E.g. SES is already hosted at jsdeliver via https://cdn.jsdelivr.net/npm/ses and unpkg here https://unpkg.com/ses

They work great too because they allow you to import deep paths, e.g. https://unpkg.com/ses/dist/ses-shim.js, or with versions/tags like https://unpkg.com/ses@0.3.0, or even combinations of both https://unpkg.com/ses@0.3.0/dist/ses-shim.js. Jsdelivr works the same.

By default they will use the browser field in your package.json, so as long as that is set for the browser build (which this PR does) then there is nothing else you need to do.

@katelynsills katelynsills self-assigned this Feb 26, 2019
Copy link
Contributor

@katelynsills katelynsills left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome! Thanks so much for doing this.

Just a couple of changes:

  • Let's remove the dist/ files and also .gitignore them.
  • Let's add the code that generates them into the 'npm run build' path
  • We want all PRs to meet our eslint config, so please run 'npm run lint-fix' before submitting

rollup.config.js Outdated

export default {
const deafultConfig = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: s/defultConfig/defaultConfig

Gitignore dist, remove dist from git tracking, fix typo, add rollup to lint and run lint
@steve8708
Copy link
Author

@katelynsills no prob, updated!

@katelynsills
Copy link
Contributor

Thanks so much for the changes! Based on your feedback, I'm creating dist/ files in some of the other packages that SES uses, and testing those out. I think SES will be a bit more complicated given that it does some unusual stuff, but we should be able to get this merged in soon!

@warner
Copy link
Member

warner commented Feb 28, 2019

yeah, SES is unusual because there's a src/stringifiedBundle file that is generated (with rollup) from the sources in src/bundle/*.js, and this file must be importable by src/index.js. This provides the SES object with a way to inject the source code that builds a SES object into a new SES realm. There is currently some linkage between the outer and inner SES realms that makes it nontrivial to avoid this (although we're trying to think of a way). So for now, we need to generate this bundle during the build process, rather than at link time. We have a separate package.json command named build-intermediate to make this, because we also need to build it before running tests or doing local development (even when the tests or local work imports the es6 module form of the code, to get the changes into a child realm's SES object, we have to rebuild the bundle first).

Kind of a drag. It might be possible to just take the published single-file dist/ses.esm.js file and pass it into s.evaluate() (maybe via our new s.makeRequire() helper somehow), but I don't expect it to work right away (a quick test I did just now fails, because of some function renaming that rollup is doing). If we could pull that off, then we might not need to build the intermediate bundle ahead of time.

@katelynsills
Copy link
Contributor

Hi Steve @steve8708, thanks so much for submitting this PR and getting us started on making SES work with bundlers. PR #97 adds dist files to SES based on the concepts on this PR and also adds bundler integration tests so that we can hopefully catch issues that may arise before they start affecting people. I hope this helps, and please let us know if you run across any other issues!

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