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

Upgrade to Babel 6 #15

Merged
merged 1 commit into from
May 13, 2016
Merged

Upgrade to Babel 6 #15

merged 1 commit into from
May 13, 2016

Conversation

doug-wade
Copy link
Contributor

This allows us to build lazyload with babel 6

@@ -260,5 +260,3 @@ LazyLoad.defaultProps = {
};

export default LazyLoad;

export lazyload from './decorator';
Copy link
Contributor Author

@doug-wade doug-wade Apr 26, 2016

Choose a reason for hiding this comment

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

As far as I can tell, the default key from this file (export default LazyLoad) is being overwritten by the default key being exported from './decorator'. By importing from react-lazyload/decorator directly, we don't have contention for the key.

My theory is that babel makes exports a single object that is returned, such that if you then re-export default something export default, you overwrite the default key of the returned object.

foo.js

export default () => console.log('foo');

bar.js

export default () => console.log('bar');
// would return { default: () => console.log('bar') }

export foo from foo;
// now returns { default: () => console.log('foo') }

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about

// src/index.js
import decorator from './decorator';
export const lazyload = decorator;

API changes are always sore spot for module consumers.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 94.538% when pulling 483f65f on redfin:upgrade-babel into 4923277 on jasonslyvia:master.

"babel-eslint": "^6.0.2",
"babel-loader": "~5.3.2",
"babel-loader": "~6.2.4",
"babel-plugin-transform-decorators-legacy": "^1.3.4",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what is recommended by Babel

@doug-wade
Copy link
Contributor Author

I had to introduce a breaking api change, changing the import for the decorator from import {lazyloader} from 'react-lazyloader' => import {decorator} as lazyloader from 'react-lazyloader/decorator'. I should test with a reduced use case against Babel 6, check the spec, and maybe submit a pr with a broken test to babel.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 94.538% when pulling 587fc85 on redfin:upgrade-babel into 4923277 on jasonslyvia:master.

@jasonslyvia
Copy link
Collaborator

Wow that's a lot of stuff, I need some time to dive into every piece of your comment.

@jasonslyvia
Copy link
Collaborator

Can you squash your commit and leave out lib/ changes for better diff?

@doug-wade
Copy link
Contributor Author

doug-wade commented May 9, 2016

@jasonslyvia Some great points; sorry for the long turnaround on feedback. I think this is much more reasonable pr. I'm also sorry for shoving a bunch of stuff into one pr; we did a lot of iteration before we got this working internally, and I didn't want to open a PR until it was clear we'd actually be relying on it, and our changes would work.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 94.8% when pulling 6fa2388 on redfin:upgrade-babel into d7e67f5 on jasonslyvia:master.

@@ -0,0 +1,4 @@
{
"presets": ["stage-0", "es2015"],
"plugins": ["transform-react-jsx", "transform-decorators-legacy"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why use transform-react-jsx plugin instead of react preset?

@doug-wade doug-wade force-pushed the upgrade-babel branch 2 times, most recently from c7ef3ab to 9acdd21 Compare May 12, 2016 23:32
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 94.8% when pulling 0155501 on redfin:upgrade-babel into 91dea19 on jasonslyvia:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 94.8% when pulling 0155501 on redfin:upgrade-babel into 91dea19 on jasonslyvia:master.

@jasonslyvia
Copy link
Collaborator

I think we're good to go as per this PR, is there further modification you're going to make, or should I merge it now?

@doug-wade
Copy link
Contributor Author

I think its ready to merge! Thanks for being so patient.

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