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.

Sign up for GitHub

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

Allow dynamic import for Node.js >=12.17 <13 || >=13.2 #256

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow dynamic import for Node.js >=12.17 <13 || >=13.2 #256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions lib/rules/no-unsupported-features/es-syntax.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

const { rules: esRules } = require("eslint-plugin-es")
const { getInnermostScope } = require("eslint-utils")
const { Range } = require("semver") //eslint-disable-line no-unused-vars
const { Range } = require("semver")
const getConfiguredNodeVersion = require("../../util/get-configured-node-version")
const getSemverRange = require("../../util/get-semver-range")
const mergeVisitorsInPlace = require("../../util/merge-visitors-in-place")
Expand Down Expand Up @@ -378,7 +378,7 @@ const features = {
ruleId: "no-dynamic-import",
cases: [
{
supported: null,
supported: new Range(">=12.17 <13 || >=13.2"),
Copy link
Author

@cognisent cognisent Feb 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I thought through several ways of accomplishing this, and landed on allowing case.supported to be a Range instance. Here are my reasons:

  • When it's a string, the getSemverRange function is called from isNotSupportingVersion while automatically prepending a < character, to make it a negative assertion to match the Not in the function name, so I'd have to mess with that behavior. I can't change getSemverRange because it's nicely memoized and used all over the place in different ways.
  • I thought of making supported allow arrays, but then exactly how to combine the array values wasn't clear.
  • Given those things, I figured that we could just say, "If you want to specify some highly-specific range, just go ahead and make a Range and we'll use that."

messageId: "no-dynamic-import",
},
],
Expand Down Expand Up @@ -453,10 +453,15 @@ function defineVisitor(context, options) {
* @returns {boolean} `true` if it's supporting.
*/
function isNotSupportingVersion(aCase) {
return (
!aCase.supported ||
options.version.intersects(getSemverRange(`<${aCase.supported}`))
)
if (!aCase.supported) {
return true
}

if (aCase.supported instanceof Range) {
return !options.version.intersects(aCase.supported)
}

return options.version.intersects(getSemverRange(`<${aCase.supported}`))
Comment on lines +456 to +464
Copy link
Author

Choose a reason for hiding this comment

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

This could be written as a single Boolean expression, but I figured expanding it was a bit more clear.

}

/**
Expand Down Expand Up @@ -646,7 +651,7 @@ module.exports = {
"no-bigint-property-names":
"Bigint literal property names are not supported yet.",
"no-dynamic-import":
"'import()' expressions are not supported yet.",
"'import()' expressions are not supported until Node.js {{supported}}. The configured version range is '{{version}}'.",
"no-optional-chaining":
"Optional chainings are not supported until Node.js {{supported}}. The configured version range is '{{version}}'.",
"no-nullish-coalescing-operators":
Expand Down
21 changes: 11 additions & 10 deletions tests/lib/rules/no-unsupported-features/es-syntax.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
const path = require("path")
const { Linter, RuleTester } = require("eslint")
const { builtin } = require("globals")
const { Range } = require("semver")
const rule = require("../../../../lib/rules/no-unsupported-features/es-syntax")

const ES2020Supported = (() => {
Expand Down Expand Up @@ -2489,27 +2490,27 @@ ruleTester.run(
code: "obj.import(source)",
options: [{ version: "12.0.0" }],
},
{
...["12.17.0", "13.2.0"].map(v => ({
Copy link
Author

Choose a reason for hiding this comment

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

In both cases, I did this spread-from-map approach to keep the object DRY, but we could also just duplicate the valid and invalid cases.

code: "import(source)",
options: [
{ version: "13.1.0", ignores: ["dynamicImport"] },
],
},
options: [{ version: v }],
})),
],
invalid: [
{
...["12.16.0", "13.0.0", "13.1.0"].map(v => ({
code: "import(source)",
options: [{ version: "13.3.0" }],
options: [{ version: v }],
errors: [
{
messageId: "no-dynamic-import",
data: {
supported: null,
version: "13.3.0",
supported: new Range(
">=12.17 <13 || >=13.2"
).toString(),
Comment on lines +2506 to +2508
Copy link
Author

Choose a reason for hiding this comment

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

I figured it was better to configure this with whatever Range's behavior is so that we didn't end up being brittle around Range when we really just wanted to test our own code.

version: v,
},
},
],
},
})),
],
},
{
Expand Down