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

Chore: First ES6 refactoring (refs #6407) #6570

Merged
merged 1 commit into from Jul 18, 2016
Merged

Chore: First ES6 refactoring (refs #6407) #6570

merged 1 commit into from Jul 18, 2016

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Jun 30, 2016

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

Refs #6407

@eslintbot
Copy link

LGTM

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @mysticatea, @vitorbal and @IanVS to be potential reviewers

@eslintbot
Copy link

LGTM

1 similar comment
@ilyavolodin
Copy link
Member

LGTM

@nzakas
Copy link
Member Author

nzakas commented Jun 30, 2016

Looks babelify isn't working on Travis...no idea why that would be. :(

@mysticatea
Copy link
Member

es2015 preset doesn't have polyfills. It has only syntax transformations. So Map is not defined.
It needs babel-runtime and babel-plugin-transform-runtime, I think.

@mikesherov
Copy link
Contributor

mikesherov commented Jun 30, 2016

babel-runtime isn't a full polyfill. It doesn't have any of the es6 instance methods. I recommend using the babel-polyfill.

@hzoo
Copy link
Member

hzoo commented Jun 30, 2016

Like @mysticatea said, Babel only transforms the syntax so Map and Set would be polyfilled.

transform-runtime is a babel plugin that finds all references to things that can be substituted with the runtime (core-js) such as Object.assign, Promise, Map, Set and doesn't change any globals (good for a library).

// 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 (String.prototype.includes, etc) so you would want to load the polyfill for that (which does set prototypes). It may be simpler to just include the polyfill (only in the tests/demo page). Consumers of the browser version would have to provide the polyfill.

For more context: http://babeljs.io/docs/plugins/transform-runtime/

@mikesherov
Copy link
Contributor

Well, not only simpler, but also actual es6.

@nzakas
Copy link
Member Author

nzakas commented Jul 1, 2016

Can someone explain what changes I have to make to this PR? How would use transform-runtime with babelify?

@hzoo
Copy link
Member

hzoo commented Jul 1, 2016

I think we want to go with the polyfill: You'll want to add babel-polyfill in the dependencies and then also add that as an input/entry to eslint.js in browserify - https://babeljs.io/docs/usage/polyfill/

If we still want to go with transform-runtime, you'll need to add babel-runtime, and babel-plugin-transform-runtime and then add that to the babel config and since it's using the cli it would be --plugins [transform-runtime] similar to what was done with the preset --presets [ es2015 ]

@eslintbot
Copy link

LGTM

@nzakas
Copy link
Member Author

nzakas commented Jul 1, 2016

Hmm, it looks like babel-polyfill means I'd have to put require("babel-polyfill") somewhere in order to use it? I'd like to avoid anything mentioning babel in the code.

I've tried using transform-runtime instead.

@mikesherov
Copy link
Contributor

You can put the polyfill only in the karma run if you'd like. babel-runtime won't polyfill instance methods.

@nzakas
Copy link
Member Author

nzakas commented Jul 2, 2016

Now I understand all the gripes about Babel configuration. :-/

@hzoo
Copy link
Member

hzoo commented Jul 2, 2016

😄 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

@nzakas
Copy link
Member Author

nzakas commented Jul 4, 2016

Hmm, we can't just use babel-polyfill in Karma, because it also must be present for the demo to work. Is there a way to use babel-polyfill without explicitly doing require("babel-polyfill")?

@hzoo
Copy link
Member

hzoo commented Jul 4, 2016

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

@nzakas
Copy link
Member Author

nzakas commented Jul 4, 2016

Hmmm, that's interesting. Maybe instead of trying to browserify babel-polyfill, I could just download that one and concatenate it into the final file.

@hzoo
Copy link
Member

hzoo commented Jul 4, 2016

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

@eslintbot
Copy link

LGTM

@nzakas
Copy link
Member Author

nzakas commented Jul 4, 2016

Okay, this seems to be what we need/want.

@hzoo
Copy link
Member

hzoo commented Jul 4, 2016

You can remove the runtime-transform/runtime now as well

@eslintbot
Copy link

LGTM

@nzakas
Copy link
Member Author

nzakas commented Jul 5, 2016

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 {
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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 // ...
  }
}

@mikesherov
Copy link
Contributor

Changes LGTM other than babel-runtime, rest of my comments are but humble suggestions.

@@ -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);
Copy link
Contributor

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.

Copy link
Member Author

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.

@eslintbot
Copy link

LGTM

@nzakas
Copy link
Member Author

nzakas commented Jul 15, 2016

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.

@hzoo
Copy link
Member

hzoo commented Jul 15, 2016

👍 , and can remove the babel-runtime devDependency in pacakge.json like mentioned before

@nzakas
Copy link
Member Author

nzakas commented Jul 18, 2016

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
@eslintbot
Copy link

LGTM

@nzakas nzakas merged commit c64b0c2 into master Jul 18, 2016
@nzakas nzakas deleted the issue6407 branch July 18, 2016 18:04
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants