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

fix: output umd build for main key #19

Merged
merged 7 commits into from
Mar 19, 2020

Conversation

keithamus
Copy link
Member

I did not notice in #15 but that PR ended up producing a build that only included ES modules.

The main key in a package.json should not be used for files that produce es modules. We should instead produce a umd compatible bundle, and link main to that.

This PR changes the build step to produce both usm and es style modules, and changes the main key to point to the umd build.

@keithamus keithamus requested a review from a team November 7, 2019 16:01
@muan
Copy link
Contributor

muan commented Nov 7, 2019

When we paired it was consciously decided that umd isn't useful to us anymore. Is satisfying what a main field expects the only reason to add it back? Is there anyway to work around that without creating a build file that we don't consume but need to maintain?

@keithamus
Copy link
Member Author

We could omit the main field entirely, and rely only on the module key.

@dgraham
Copy link
Contributor

dgraham commented Nov 7, 2019

Something to look forward to is Node's experimental support for a "type": "module" key in package.json. Once that stabilizes, we'll point main to the module file and won't need to build UMD files.

It looks like producing UMD output is fairly easy to do with command line overrides to tsc, so it would be easy to remove when we start using type: module.

Copy link
Contributor

@muan muan left a comment

Choose a reason for hiding this comment

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

I don’t have strong opinions against this. Feel free to carry on. Not sure why the tests are failing though.

Copy link
Contributor

@koddsson koddsson 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 good to me 🎇

},
'test.js'
],
files: ['bootstrap.js', '../dist/umd/index.js', 'test.js'],
Copy link
Contributor

Choose a reason for hiding this comment

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

bootstrap.js seems like an over-complication in comparison to loading the native module. Can we keep the test using the module output here?

@keithamus keithamus merged commit 3e28340 into master Mar 19, 2020
@keithamus keithamus deleted the fix-output-umd-build-for-main-key branch March 19, 2020 13:34
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.

4 participants