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
Chore: First ES6 refactoring (refs #6407) #6570
Conversation
LGTM |
By analyzing the blame information on this pull request, we identified @mysticatea, @vitorbal and @IanVS to be potential reviewers |
LGTM |
1 similar comment
LGTM |
Looks babelify isn't working on Travis...no idea why that would be. :( |
es2015 preset doesn't have polyfills. It has only syntax transformations. So Map is not defined. |
babel-runtime isn't a full polyfill. It doesn't have any of the es6 instance methods. I recommend using the babel-polyfill. |
Like @mysticatea said, Babel only transforms the syntax so
// simplified output
// in
new Promise();
// out
var _promise = require("babel-runtime/core-js/promise");
new _Promise(); However like @mikesherov mentioned it doesn't change instance methods because we can't be confident it's the right one ( For more context: http://babeljs.io/docs/plugins/transform-runtime/ |
Well, not only simpler, but also actual es6. |
Can someone explain what changes I have to make to this PR? How would use |
I think we want to go with the polyfill: You'll want to add If we still want to go with transform-runtime, you'll need to add |
LGTM |
Hmm, it looks like I've tried using |
You can put the polyfill only in the karma run if you'd like. babel-runtime won't polyfill instance methods. |
Now I understand all the gripes about Babel configuration. :-/ |
😄 You can use babel 5 if you want less "config". Either way babel-polyfill is a separate thing (useshttps://github.com/zloirock/core-js) since babel only does syntax |
Hmm, we can't just use |
You can also load babel-polyfill in the demo page separately? There's a dist folder so you could add https://npmcdn.com/babel-polyfill@6.9.1/dist/polyfill.js in a script tag |
Hmmm, that's interesting. Maybe instead of trying to browserify |
Ah yeah I think that's what it says in the docs: https://babeljs.io/docs/usage/polyfill/ Either require in browserify (top entry point), or prepend it or add in a script tag |
LGTM |
Okay, this seems to be what we need/want. |
You can remove the runtime-transform/runtime now as well |
LGTM |
Okay, I think I got it finally |
@@ -30,28 +30,28 @@ var RULE_SEVERITY_STRINGS = ["off", "warn", "error"], | |||
// Public Interface | |||
//------------------------------------------------------------------------------ | |||
|
|||
module.exports = { | |||
module.exports = class ConfigOps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May want to consider not making this into a class. A class whose only members are static functions is just a fancy way of saying object
.
Personally I find all the extra static
keywords to be noisy.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm much more of the mindset that classes with static methods are better than objects when there is no internal state (which is to say, it's a namespace). Happy to defer to consensus, though, as this was really more of a forcing function to get the Babel setup working rather than anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that modules are good namespaces in ES6. Anyhow, you can still use the "class-style" function declaration in objects, maybe something to consider.
e.g.
module.exports = {
createEmptyConfig () {
return // ...
}
}
Changes LGTM other than |
@@ -159,7 +159,7 @@ module.exports = { | |||
if (isRule) { | |||
dst[i] = e; | |||
} else { | |||
dst[i] = deepmerge(target[i], e, combine, isRule); | |||
dst[i] = ConfigOps.merge(target[i], e, combine, isRule); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you avoid using this
here to avoid confusion with mistaking what this
refers to in a static context? Just curious more than anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish I could claim I did it for some smart reason, but I just did it without thinking. this
would work equally well.
LGTM |
I reverted back to using object literals instead of classes and instead used shorthand methods to verify that ES6 syntax doesn't break the tests. |
👍 , and can remove the |
Dammit, I could have sworn I did that. :-X |
First steps in taking advantage of ES6 features: 1. Remove es6-map dependency (no longer needed) 2. Use babelify to create browser bundle 3. Refactor a couple files into ES6 classes
LGTM |
First steps in taking advantage of ES6 features:
Refs #6407