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

Resolve deprecation warning message from eslint while running eslint #1586

Merged
merged 4 commits into from Oct 28, 2019

Conversation

@kabirbaidhya
Copy link
Contributor

@kabirbaidhya kabirbaidhya commented Oct 28, 2019

Issue

Running grunt eslint or ESLint CLI directly throws this deprecation warning since it was configured as per the older version of eslint:

(node:7146) DeprecationWarning: [eslint] The 'ecmaFeatures' config file property is deprecated, and has no effect. (found in /home/kabir/opensource/handlebars.js/.eslintrc.js)

image

Changes

I followed this Migrating to v2.0.0 to update the configuration as per the recommendation which fixed the deprecation message.

  • The ecmaFeatures property is now under a top-level parserOptions property.
  • Define "ecmaVersion": 6 since we're using ES6.
  • Enable ES6 built-in global variables by adding "es6": true under env.
  • Remove all the EcmaScript 6 flags that are now already available with ecmaVersion: 6. Check the ESLint migration guidelines for details - https://eslint.org/docs/user-guide/migrating-to-2.0.0#language-options.

Now

After these changes you shouldn't see the DeprecationWarning like before.
image

You can also check the travis build logs - for 4.x branch and for the PR 1586 and see that the warning isn' showing up now.


Before creating a pull-request, please check https://github.com/wycats/handlebars.js/blob/master/CONTRIBUTING.md first.

Generally we like to see pull requests that

  • Please don't start pull requests for security issues. Instead, file a report at https://www.npmjs.com/advisories/report?package=handlebars
  • Maintain the existing code style
  • Are focused on a single change (i.e. avoid large refactoring or style adjustments in untouched code if not the primary goal of the pull request)
  • Have good commit messages
  • Have tests
  • Have the typings (lib/handlebars.d.ts) updated on every API change. If you need help, updating those, please mention that in the PR description.
  • Don't significantly decrease the current code coverage (see coverage/lcov-report/index.html)
  • Currently, the 4.x-branch contains the latest version. Please target that branch in the PR.
There was a DeprecationWarning message from eslint saying that `ecmaFeatures` property has been deprecated.
Moved it under the `parserOptions` as per recommended here - https://eslint.org/docs/user-guide/migrating-to-2.0.0.
@kabirbaidhya kabirbaidhya marked this pull request as ready for review Oct 28, 2019
@kabirbaidhya kabirbaidhya changed the title Resolve deprecation warning message from eslint while running Eslint CLI Resolve deprecation warning message from eslint while running eslint Oct 28, 2019
@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 28, 2019

The comment in the eslint config indicates that not all es6 features should be enabled.

Object.assign for example does not work in IE and we don't use polyfills.

Its a good thing to fix the config. Could you do it so that the list of features is used again?

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 28, 2019

It would also be helpful to have the disable es6-language at all for the files in the spec directory. Those are not compiled with babel. I am planning to refactor the tests, but until than, that would be helpful.

It has happend a couple of times that I wrote a test and everything was find when I ran the tests locally. But when they were executed in SauceLabs with IE11, there was an error.

It doesn't have to be in this PR, but it is something to keep in mind.

@kabirbaidhya
Copy link
Contributor Author

@kabirbaidhya kabirbaidhya commented Oct 28, 2019

Hi @nknapp,
Did you mean to undo these list of ecma script features?
image

Actually, they aren't being removed; but it's just that explicitly setting them under ecmaFeatures isn't required anymore in the current version of eslint - these are already included when you set ecmaVersion: 6.

So, that being said there isn't actually any change done in the config. These are default features of Ecma Script that eslint already knows once you set the version now. Check this:
image

Also, as per the deprecation warning - even if you do set them it has no effect now.

(node:7146) DeprecationWarning: [eslint] The 'ecmaFeatures' config file property is deprecated, and has no effect. (found in /home/kabir/opensource/handlebars.js/.eslintrc.js)

Let me know your thoughts on this.

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 28, 2019

Are there any features that are allowed by eslint after your change that weren't allowed when the ecmaFeatures property was still used by the old eslint-version?

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 28, 2019

I think I misunderstood what the "ecmaFeatures" was doing exactly. But still rules for avoiding "Object.assign" and other things that break in IE or Safari, would be great.

That's another matter however.

@nknapp nknapp merged commit 7052e88 into handlebars-lang:4.x Oct 28, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 28, 2019

I have added the eslint-plugin-compat, which checks for browse-compatibility and eslint-plugin-es5 which is now configured to ensure that the spec/ directory only contains es5 syntax. (The reason why the tests have failed after merging #1584)

@kabirbaidhya kabirbaidhya deleted the kabirbaidhya:eslint-deprecation branch Oct 29, 2019
@kabirbaidhya
Copy link
Contributor Author

@kabirbaidhya kabirbaidhya commented Oct 29, 2019

Hi @nknapp, Thanks!

Regarding your comment:

But still rules for avoiding "Object.assign" and other things that break in IE or Safari, would be great.

I'll check if there are rules available for these use cases and can do a PR if possible.

@nknapp
Copy link
Collaborator

@nknapp nknapp commented Oct 29, 2019

Eslint-plugin-compat is doing that. Check for Object.assign etc.
I've added it already, because the last three builds already failed because of a "let" in the tests, which I had not seen in my review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.