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

Prefer JavaScript UMD modules spec #1028

Merged

Conversation

jackturnbull
Copy link
Contributor

Description of the Change

It appears that Webpack is already handling the UMD import/export specification correctly when compiling the assets within Amber. In the amber.js websocket file, both UMD and common JS are present, and non-identical as both class Channel and class Socket are exported in the UMD format but only Socket is exported as CommonJS. A side-effect of this is that the module.exports object is transpiled into the bundle as Webpack has already defaulted to using UMD.

This commit simply removes the CommonJS export definition.

Alternate Designs

None / N/A

The CommonJS pattern could be chosen, but with Webpack support for UMD I don't believe this would be worthwhile.

Benefits

This was spotted as I had changed my JavaScript bundler to Rollup.js but the output was evidently not handled as graciously as Webpack's as I had a module.exports remaining in the output without module itself being defined.

The benefit being that this opens the floor to using other JS bundlers that also contain the same issue.

Possible Drawbacks

None as far as I can tell. I've stepped through the output of the webpack bundles before and after this change and in both cases both the Socket class and the Channel class are the only two modules passed into the bootstrap function, although if anyone has more context for why the module.exports definitions remain that would help also!

Summary of the output bundles:

  • Before
    • Defined on module: Socket
    • Defined on exports: Socket, Channel
    • Passed to boostrap function: Socket, Channel (Socket deduped)
  • After
    • Defined on module:
    • Defined on exports: Socket, Channel
    • Passed to boostrap function: Socket, Channel

@jackturnbull
Copy link
Contributor Author

Waiting on #1027 to get the tests passing

drujensen
drujensen previously approved these changes Jan 7, 2019
Copy link
Member

@drujensen drujensen left a comment

Choose a reason for hiding this comment

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

👍

It appears that Webpack is already handling the UMD import/export specification correctly when compiling the assets within Amber. In the amber.js websocket file, both UMD and common JS are present, and non-identical as both `class Channel` and `class Socket` are exported in the UMD format but only Socket is exported as CommonJS. A side-effect of this is that the `module.exports` object is transpiled into the bundle as Webpack has already defaulted to using UMD.

This commit simply removes the CommonJS export definition.
@jackturnbull
Copy link
Contributor Author

Sorry, appears that the rebase has squashed the revew! Tests passing at least.

Copy link
Member

@drujensen drujensen left a comment

Choose a reason for hiding this comment

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

First contribution! 🥇

Copy link
Member

@robacarp robacarp left a comment

Choose a reason for hiding this comment

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

great, thanks @jackturnbull!

@robacarp robacarp merged commit 238d5a0 into amberframework:master Jan 8, 2019
@jackturnbull jackturnbull deleted the bug/single-js-export-format branch January 8, 2019 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants