Dependency Extraction Webpack Plugin: Add types #22498
Conversation
sirreal
added
[Status] In Progress
sirreal
force-pushed
the
update/type-webpack-dependency-extraction-plugin
branch
from
0b16094
to
16f20bb
Compare
|
Size Change: 0 B Total Size: 1.12 MB ℹ️ View Unchanged
|
|
There's a fix for unrelated lint errors in #22785. |
sirreal
force-pushed
the
update/type-webpack-dependency-extraction-plugin
branch
from
ffc9eeb
to
e57cb92
Compare
| // Use the default eslint parser. Prevent babel transforms. | ||
| "parser": "espree", |
There was a problem hiding this comment.
Is this to address the issue of the optional chaining false negative?
Alternatively, does the project-default babel-eslint parser respect parserOptions like ecmaVersion ? Still an imperfect solution, since it's hard to know how to associate "Node LTS 12.x" with a specific ecmaVersion value.
There was a problem hiding this comment.
Is this to address the issue of the optional chaining false negative?
Exactly.
Alternatively, does the project-default babel-eslint parser respect parserOptions like
ecmaVersion? Still an imperfect solution, since it's hard to know how to associate "Node LTS 12.x" with a specific ecmaVersion value.
I don't know, I played with a few iterations of this and settled on one that appeared to work. I'm open to suggestions. It's probably worth solving at a repo-wide level as I believe other Node targeting packages are affected, e.g. project-management-automation may be susceptible to introduction of unsupported optional chaining as well. I believe the unit tests were passing for this, which suggests that the Jest setup may be transforming Node files and hiding the issue. I haven't confirmed.
There was a problem hiding this comment.
It's probably worth solving at a repo-wide level as I believe other Node targeting packages are affected, e.g. project-management-automation may be susceptible to introduction of unsupported optional chaining as well.
Yeah, I've definitely encountered this myself, where it appears all well in the editor, but fails at runtime. I agree it could use a common solution amongst non-transpiled packages. Fine to leave it as a separate task if you'd rather it not block here.
There was a problem hiding this comment.
I'm tinkering with this a bit locally. ecmaVersion alone doesn't seem to help much with babel-parser. I'd still think it might be worth specifying, depending if the default parser suddenly becomes updated to support optional chaining (possibly happening in #22771 with the ESLint 7.0 upgrade?)
There was a problem hiding this comment.
Thinking about it a bit more, using the default espree parser might be an objectively good way to configure ESLint in packages which aren't processed by Babel, if one considers the need for babel-eslint to align to expectations of how the source code is transformed.
It should still make sense that the parser be configured to expectations of the Node runtime we're expecting.
The alternative would seem to be that we'd keep babel-eslint but find some way to "disable" plugins for language syntax we don't want to apply to packages which aren't processed by Babel. Doesn't seem like it's an especially simple task to accomplish.
There was a problem hiding this comment.
Fine to leave it as a separate task if you'd rather it not block here.
There's no hurry to get this merged. I believe the package changes are finalized and I'm happy to leave it open to tweak the eslint setup. Would you like to take it over figuring out the eslint setup and merge when you're satisfied? My only concern is a lack of clarity leading the the PR being forgotten.
Thinking about it a bit more, using the default
espreeparser might be an objectively good way to configure ESLint in packages which aren't processed by Babel, if one considers the need forbabel-eslintto align to expectations of how the source code is transformed.
This line of thinking makes a lot of sense to me. I didn't get into the pareserOptions/ecmaVersion because, as you say, those aren't tied to Node versions.
There was a problem hiding this comment.
I've observed a lot of flakiness in the e2e tests and with them passing I'm going to go ahead and merge this 🙂
sirreal
force-pushed
the
update/type-webpack-dependency-extraction-plugin
branch
from
9b9a7b3
to
a44820e
Compare
sirreal
force-pushed
the
update/type-webpack-dependency-extraction-plugin
branch
from
a44820e
to
b6a9bb1
Compare
Description
?.without producing any errors or warnings. Fortunately, the issue surfaced unexpectedly in the size check action.This would be a good idea for all of the node-focused packages.
Part of #18838
How has this been tested?
Manual and automated testing of the plugin.
Types of changes
New feature: Include and publish TypeScript types.
Checklist: