fix(pluginutils): attach scope object to for-loop#616
fix(pluginutils): attach scope object to for-loop#616shellscape merged 3 commits intorollup:masterfrom
Conversation
lukastaegert
left a comment
lukastaegert
left a comment
There was a problem hiding this comment.
Looks good, just some minor type-related comments
| } | ||
| `, | ||
| { ecmaVersion: 2020, sourceType: 'module' } | ||
| ) as unknown) as import('estree').Program; |
There was a problem hiding this comment.
Wouldn't it be nicer to just
import {Program, ForStatement,...} from 'estree'once at the top?
There was a problem hiding this comment.
Also, shouldn't this make "estree" and explicit devDependency in the package.json file?
There was a problem hiding this comment.
I'm not familiar with typescript so these were copied from:
Should we also move them to the top?
There was a problem hiding this comment.
Ah I see, I did not notice that one. So Node is special in so far as it is also a builtin type so one should be careful to override it, this can cause confusion. Maybe in that case just use a star import instead (works for types just like regular things):
import * as estree from 'estree';
and then below you use e.g. estree.Node or estree.ForStatement. Also do not forget to install estree es devDependency, which you should do via pnpm install -D estree from the package directory so that the lockfile is updated.
| t.falsy(scope.contains('a')); | ||
| t.falsy(scope.contains('b')); | ||
|
|
||
| const forLoop = ast.body[0] as import('estree').ForStatement & Record<string, any>; |
There was a problem hiding this comment.
How about as ForStatement & {scope: AttachedScope}, using the type that attachScopes itself uses?
| "@rollup/plugin-commonjs": "^14.0.0", | ||
| "@rollup/plugin-node-resolve": "^8.4.0", | ||
| "@rollup/plugin-typescript": "^5.0.2", | ||
| "@types/estree": "0.0.45", |
There was a problem hiding this comment.
I ended up adding @types/estree instead of estree
| // eslint-disable-next-line import/no-unresolved | ||
| import * as estree from 'estree'; | ||
|
|
||
| import { walk } from 'estree-walker'; |
There was a problem hiding this comment.
This was copied from
plugins/packages/pluginutils/types/index.d.ts
Lines 1 to 2 in dbececf
lukastaegert
left a comment
lukastaegert
left a comment
There was a problem hiding this comment.
Looks good to go from my side!
|
|
3 participants
Rollup Plugin Name:
pluginutilsThis PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers: Fixes #547, fixes #548, fixes #549
Description
This PR adds a new behavior to
attachScopes. It creates a new scope object forForStatement,ForInStatement, andForOfStatement. IDK if this should be considered as a breaking change.