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

Feature/272 es6 pkg module #275

Merged
merged 7 commits into from
Nov 27, 2017
Merged

Feature/272 es6 pkg module #275

merged 7 commits into from
Nov 27, 2017

Conversation

pglewis
Copy link
Contributor

@pglewis pglewis commented May 31, 2017

Fixes #272

  • Gets rid of implicit global warnings in the Rollup build
  • Updates Babel and Rollup related packages
  • Generates a .esm.js module and map file

@blikblum
Copy link
Contributor

It's failing due to webpack version.

As a side note, later, we should replace webpack by rollup

@pglewis
Copy link
Contributor Author

pglewis commented May 31, 2017

I wonder if just rolling back babel-loader to 6.2.0 would be enough to iron it out. That one isn't involved in the build so I probably should have left it alone.

Copy link
Member

@scott-w scott-w left a comment

Choose a reason for hiding this comment

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

I'm a fan and I love seeing work being done to keep the package up-to-date with the latest build tools.

I just have a question around artefacts as I'm a bit behind on how es6 package modules work.

@@ -4,6 +4,7 @@
"homepage": "https://github.com/marionettejs/backbone.radio",
"version": "2.0.0",
"main": "build/backbone.radio.js",
"module": "build/backbone.radio.esm.js",
Copy link
Member

Choose a reason for hiding this comment

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

Does this file need to be checked into the source tree or is it automatically generated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes, I should have added the module and source map to git

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's wired into the build so would get generated with the rest of the files in build/ in the future.

I'm not sure what needs to be done, if anything for the NPM package to include the ES modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

The es build must be in the package distributed.

Commit in the repository is optional. Anyway if deciding to commit to repository, It should be done only just before a release.

function build(done) {
rollup.rollup({
var banner = getBanner();
Copy link
Member

Choose a reason for hiding this comment

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

this line doesn't appear to be in use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, detritus. Yanked in 8f563e5

@paulfalgout
Copy link
Member

@marionettejs/marionette-core if 2nd thumb'd squash and merge

@paulfalgout
Copy link
Member

We need to make sure this has named exports

@paulfalgout paulfalgout added this to the v3 milestone Nov 4, 2017
@blikblum
Copy link
Contributor

blikblum commented Nov 26, 2017

I think it can be merged as is.

The addition of named exports can be added later since means a bigger change and needs some discussion.

Radio public API is basically Channel class, channel function , reset, tuneIn, tuneOut, log, debugLog

While in Marionette migrating everything to named exports and killing the default exports was straight, in Radio doing the same will be a big breaking change.

By far the most used API in channel function, used like:

import Radio from 'backbone.radio'

Radio.channel('yy').request('xx')

By migrating to named exports like it would need to be changed to

import {channel} from 'backbone.radio'

channel('yy').request('xx')

There's some issues:

  • channel is not a good name for a function, looks more like a variable
  • channel and Channel would be exported, adding confusion
  • debugLog could not be override

So migrating to named exports is possible but will need some API design changes that at this time (just before releasing marionette v4) i don't think is doable.

I propose to merge this and open a new issue for named export

@blikblum blikblum mentioned this pull request Nov 27, 2017
@blikblum blikblum merged commit 72ed976 into marionettejs:master Nov 27, 2017
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.

None yet

4 participants