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

Add node/no-unsupported-features rules and fix unit tests #341

Merged
merged 1 commit into from
Feb 18, 2020

Conversation

pvdlg
Copy link
Contributor

@pvdlg pvdlg commented Jul 22, 2018

Fix #108

The following rules are added, only if the node-version is used or if the package.json has a valid engines.node prop:

  • node/no-unsupported-features/es-builtins
  • node/no-unsupported-features/es-syntax
  • node/no-unsupported-features/node-builtins

@pvdlg pvdlg requested a review from sindresorhus July 22, 2018 04:33
@pvdlg
Copy link
Contributor Author

pvdlg commented Jul 22, 2018

@sindresorhus it seems the repo is not activated on Travis anymore. Maybe the orga has been migrated to travis.com?

@sindresorhus
Copy link
Member

This is a great addition, I'm just reluctant to merge it because it's going to cause a lot of problems for me. I use async functions with AVA in lots of tests (that are transpiled), even though the module targets Node.js 6. Can we {ignore: ['asyncFunctions']} for now?

This is what I'm seeing everywhere:

  ✖    7:45  Async functions are not supported until Node.js 7.6.0. The configured version range is >=6.0.0.  node/no-unsupported-features/es-syntax
  ✖   20:43  Async functions are not supported until Node.js 7.6.0. The configured version range is >=6.0.0.  node/no-unsupported-features/es-syntax
  ✖   46:27  Async functions are not supported until Node.js 7.6.0. The configured version range is >=6.0.0.  node/no-unsupported-features/es-syntax
  ✖   53:38  Async functions are not supported until Node.js 7.6.0. The configured version range is >=6.0.0.  node/no-unsupported-features/es-syntax
  ✖   62:71  Async functions are not supported until Node.js 7.6.0. The configured version range is >=6.0.0.  node/no-unsupported-features/es-syntax
  ✖   71:42  Async functions are not supported until Node.js 7.6.0. The configured version range is >=6.0.0.  node/no-unsupported-features/es-syntax
  ✖   80:62  Async functions are not supported until Node.js 7.6.0. The configured version range is >=6.0.0.  node/no-unsupported-features/es-syntax
  ✖   89:68  Async functions are not supported until Node.js 7.6.0. The configured version range is >=6.0.0.  node/no-unsupported-features/es-syntax

@sindresorhus
Copy link
Member

it seems the repo is not activated on Travis anymore. Maybe the orga has been migrated to travis.com?

It's not migrated, but I've had this issue on other repos too. Probably just another Travis bug. I've tried to reactivate the repo now, but I don't think it takes effect on existing PRs...

@pvdlg
Copy link
Contributor Author

pvdlg commented Jul 24, 2018

This is a great addition, I'm just reluctant to merge it because it's going to cause a lot of problems for me. I use async functions with AVA in lots of tests (that are transpiled), even though the module targets Node.js 6. Can we {ignore: ['asyncFunctions']} for now?

Now that you mention that, it might create other issues unless you use override to specify that everything under test should use nodeVersion: >=10.

The proper fix would be to detect AVA files and put them in an override with nodeVersion: >=10.
I think I tried that in a78e02e but we stopped as we didn't find a solid way to detect AVA files.

Maybe I can split this PR in two:

  • 1 to fix the test
  • 1 to add node/no-unsupported-features that we can merge later once we have a solution for AVA files

@pvdlg
Copy link
Contributor Author

pvdlg commented Jul 24, 2018

See #344

@sindresorhus
Copy link
Member

I think I tried that in a78e02e but we stopped as we didn't find a solid way to detect AVA files.

Was there a PR for this with a discussion? Link?

@pvdlg
Copy link
Contributor Author

pvdlg commented Jul 24, 2018

Yes: #281

@sindresorhus
Copy link
Member

I think this feature is pretty valuable. How about we do {ignore: ['asyncFunctions']} for now and merge this and then open an issue about enabling it when we have a solution for AVA?

@pvdlg
Copy link
Contributor Author

pvdlg commented Jul 28, 2018

I think this feature is pretty valuable. How about we do {ignore: ['asyncFunctions']} for now and merge this and then open an issue about enabling it when we have a solution for AVA?

I don't think it's a good idea, because even if asyncFunctions is the most common case you will have many other features/API that will be problematic.

Maybe we could just set nodeVersion to 10 for the typical AVA directory and explain in the doc that if you have any directory with files that will be transpiled (with Babel or non-default AVA directory) you have to set nodeVersion in an override for that directory. That applies only if you set engines.node in your package.json

@sindresorhus sindresorhus force-pushed the master branch 2 times, most recently from 9bbe1bf to 2ab0a69 Compare May 25, 2019 08:31
@sindresorhus
Copy link
Member

Good news. AVA no longer includes Babel by default. https://github.com/avajs/ava/releases/tag/v3.0.0

@@ -226,31 +224,32 @@ test('buildConfig: nodeVersion: false', t => {
t.is(config.rules['node/prefer-promises/fs'], 'off');
});

test('buildConfig: nodeVersion: invalid range', t => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed that test as this case can never actually happens as the validity of the semver range is verified earlier in mergeOptions

@pvdlg
Copy link
Contributor Author

pvdlg commented Feb 18, 2020

@sindresorhus I updated the PR and clarified the doc regarding transpiled code.

If you have a codebase that targets two different Node version (as it was the case with AVA) then you will need to define an overrides to avoid having the new rules reporting error.

But any case it's a good practice to define such overrides as it would enable more rules for your source (non-transpiled) code such as unicorn/prefer-flat-map, no-useless-catch, prefer-object-spread etc...

@sindresorhus
Copy link
Member

But any case it's a good practice to define such overrides as it would enable more rules for your source (non-transpiled) code such as unicorn/prefer-flat-map, no-useless-catch, prefer-object-spread etc...

👍

@sindresorhus sindresorhus merged commit 2297c07 into master Feb 18, 2020
@sindresorhus sindresorhus deleted the no-unsupported-features branch February 18, 2020 07:33
@sindresorhus
Copy link
Member

Awesome to be able to finally merge this 🙌

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.

Add no-unsupported-features
2 participants