fix(inject): detect references inside object destructuring #1241

eight04 · 2022-08-12T16:49:32Z

Rollup Plugin Name: `inject`

This PR contains:

Are tests included?

yes (bugfixes and features will not be merged without tests)
no

Breaking Changes?

yes (breaking changes will not be merged unless absolutely necessary)
no

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:

#1235

Description

Currently the plugin doesn't detect references inside object destructuring:

// `process` wasn't detected
function foo({cwd = process.cwd()}) { }

This PR fixes it.

Another issue: I ran git commit and hit this bug: pnpm/pnpm#4790
I ended up running git commit --no-verify so something might be wrong.

Also it seems that test snapshots are built differently on different platforms. You may want to rebuild it on Unix.

shellscape · 2022-08-12T18:15:51Z

What platform are you developing on? We get quite a few Windows users submitting PRs and a good chunk of the maintainers use MacOS and Linux.

eight04 · 2022-08-13T02:13:09Z

Windows 10 / Node 17.4.0.

shellscape · 2022-08-13T02:14:03Z

Maybe it's your Node version? Even Node doesn't recommend running odd number versions for anything remotely production.

eight04 · 2022-08-13T02:33:57Z

I upgraded Node.js to 18.7.0.

Still can't git commit:

C:\Users\eight04\dev\plugins\packages\inject>git commit -m test
 ERR_PNPM_FETCH_404  GET https://registry.npmjs.org/--no-install: Not Found - 404

--no-install is not in the npm registry, or you have no permission to fetch it.

An authorization header was used: Bearer ****[hidden]

Test snapshots still doesn't include escapes backslashes section.

shellscape · 2022-08-13T02:40:46Z

There's something awry on your system I'm afraid. Just cloned a fresh copy, applied your changes, and had no issues with committing. WSL might be an option for you, but generally when we see odd errors like that, it points back to bad system config.

shellscape · 2022-08-13T02:42:45Z

Out of curiosity, which version of pnpm are you using?

eight04 · 2022-08-13T02:44:08Z

pnpm 7.9.0.

Someone in the thread (pnpm/pnpm#4790) did suggest that downgrading pnpm solves the issue. Should I try that?

shellscape · 2022-08-13T02:45:06Z

Yes please try the last v6.x version. fwiw our lockfile here is still v6 format.

eight04 · 2022-08-13T02:48:21Z

Yes downgrading pnpm to 6.x works.

C:\Users\eight04\dev\plugins\packages\inject>pnpm --version
6.34.0

C:\Users\eight04\dev\plugins\packages\inject>git commit -m "test"
husky > pre-commit (node v18.7.0)
[STARTED] Preparing...
[SUCCESS] Preparing...
[STARTED] Running tasks...
[STARTED] Running tasks for *.{ts,js}
[STARTED] Running tasks for **/(package|tsconfig(.*)?).json
[STARTED] Running tasks for (pnpm-workspace|.github/**/*).{yml,yaml}
[STARTED] Running tasks for ((.github/**/*)|(README|CHANGELOG)|(**/(README|CHANGELOG))).md
[SKIPPED] No staged files match **/(package|tsconfig(.*)?).json
[SKIPPED] No staged files match (pnpm-workspace|.github/**/*).{yml,yaml}
[SKIPPED] No staged files match ((.github/**/*)|(README|CHANGELOG)|(**/(README|CHANGELOG))).md
[STARTED] eslint --cache --fix
[SUCCESS] eslint --cache --fix
[SUCCESS] Running tasks for *.{ts,js}
[SUCCESS] Running tasks...
[STARTED] Applying modifications...
[SUCCESS] Applying modifications...
[STARTED] Cleaning up...
[SUCCESS] Cleaning up...
[dev-destruct 9e8fa3a] test
 1 file changed, 1 insertion(+), 1 deletion(-)

shellscape · 2022-08-13T02:49:08Z

sweet. something still squirrelly with v7

shellscape · 2022-09-10T20:32:41Z

I'm going to be bumping the pnpm lockfile version in master on Monday (I think). we can resolve the CI issues with this PR then.

eight04 · 2022-09-11T01:34:08Z

The error is that when testing on Windows, one test snapshot is missing.

I think we need someone who dont use Windows to rebuild snapshots in this pr.

eight04 · 2022-09-11T01:35:41Z

Or fix the test so it can generate snapshots correctly on Windows.

lukastaegert · 2022-10-14T04:57:46Z

packages/inject/src/index.js

@@ -167,7 +167,7 @@ export default function inject(options) {

          // special case – shorthand properties. because node.key === node.value,
          // we can't differentiate once we've descended into the node
-          if (node.type === 'Property' && node.shorthand) {
+          if (node.type === 'Property' && node.shorthand && node.value.type === 'Identifier') {


My worry is that with this change, if the shorthand property is an assignment pattern but the left side should be replaced, it would cause the same issue we are guarding against.

i.e. what would happen here

export const test = ({process = {}}) => { console.log(process); };

(Admittedly I did not run the code...)
Maybe we should rather add special handling for assignment patterns in shorthands than restrict the current special handling to the identifier case?

I think it should be guarded by if (scope.contains(...)) since process is a local variable in this case.

I guess we can add more test cases e.g.

{ const {$} = foo; } { const foo = {$}; } { const foo = ({$}) => {}; } { const foo = ({bar = $}) => {}; }

eight04 · 2022-10-14T06:45:57Z

I tried to build this on master but then got a new error:

> @rollup/plugin-inject@5.0.0 build C:\Users\eight04\dev\rollup-plugins\packages\inject
> rollup -c

[!] Error: Debug Failure. Expected C:/Users/eight04/dev/rollup-plugins/packages/inject/tsconfig.json === C:\Users\eight04\dev\rollup-plugins\packages\inject\tsconfig.json.
Error: Debug Failure. Expected C:/Users/eight04/dev/rollup-plugins/packages/inject/tsconfig.json === C:\Users\eight04\dev\rollup-plugins\packages\inject\tsconfig.json.
    at attachFileToDiagnostic (C:\Users\eight04\dev\rollup-plugins\node_modules\.pnpm\typescript@4.8.4\node_modules\typescript\lib\typescript.js:20253:18)
    at Object.attachFileToDiagnostics (C:\Users\eight04\dev\rollup-plugins\node_modules\.pnpm\typescript@4.8.4\node_modules\typescript\lib\typescript.js:20285:42)
    at Object.parseJsonText (C:\Users\eight04\dev\rollup-plugins\node_modules\.pnpm\typescript@4.8.4\node_modules\typescript\lib\typescript.js:32087:46)
    at Object.parseJsonText (C:\Users\eight04\dev\rollup-plugins\node_modules\.pnpm\typescript@4.8.4\node_modules\typescript\lib\typescript.js:31813:23)
    at parseConfigFileTextToJson (C:\Users\eight04\dev\rollup-plugins\node_modules\.pnpm\typescript@4.8.4\node_modules\typescript\lib\typescript.js:41443:33)
    at Object.readConfigFile (C:\Users\eight04\dev\rollup-plugins\node_modules\.pnpm\typescript@4.8.4\node_modules\typescript\lib\typescript.js:41434:48)
    at readTsConfigFile (C:\Users\eight04\dev\rollup-plugins\node_modules\.pnpm\@rollup+plugin-typescript@8.5.0_typescript@4.8.4\node_modules\@rollup\plugin-typescript\dist\index.js:303:34)
    at parseTypescriptConfig (C:\Users\eight04\dev\rollup-plugins\node_modules\.pnpm\@rollup+plugin-typescript@8.5.0_typescript@4.8.4\node_modules\@rollup\plugin-typescript\dist\index.js:356:41)
    at typescript (C:\Users\eight04\dev\rollup-plugins\node_modules\.pnpm\@rollup+plugin-typescript@8.5.0_typescript@4.8.4\node_modules\@rollup\plugin-typescript\dist\index.js:758:27)
    at createConfig (file:///C:/Users/eight04/dev/rollup-plugins/shared/rollup.config.mjs:38:15)

Seems like a cross platform issue related to path separator.

lukastaegert · 2022-10-14T07:24:24Z

I think it should be guarded by if (scope.contains(...))

Fair enough

I guess we can add more test cases

Also

const {$ = fallback} = foo;

which is the example I should have written.

eight04 · 2022-10-14T12:42:19Z

Done

lukastaegert

Somehow when you recreated the snapshots, one test case was removed, I fixed this from my side by recreating them again, I wonder what caused it. (I usually do pnpm t -- -u to recreate, if you do the same, then it might be some environment thing). Looks good to be merged.

eight04 requested a review from shellscape as a code owner August 12, 2022 16:49

lukastaegert force-pushed the master branch from f336198 to daf3d4d Compare October 7, 2022 19:38

eight04 mentioned this pull request Oct 14, 2022

[inject] doesn't transform references in object destructuring #1235

Closed

lukastaegert reviewed Oct 14, 2022

View reviewed changes

Fix: doesn't detect references in destructed object value

a8a8ddc

eight04 force-pushed the dev-destruct branch from 9e25604 to a8a8ddc Compare October 14, 2022 12:11

Fix: pass the linetr

aac5424

Update snapshot

c32a7e4

lukastaegert force-pushed the dev-destruct branch from b11bbcb to c32a7e4 Compare October 15, 2022 04:23

lukastaegert approved these changes Oct 15, 2022

View reviewed changes

lukastaegert merged commit b64cd9c into rollup:master Oct 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(inject): detect references inside object destructuring #1241

fix(inject): detect references inside object destructuring #1241

eight04 commented Aug 12, 2022

shellscape commented Aug 12, 2022

eight04 commented Aug 13, 2022

shellscape commented Aug 13, 2022

eight04 commented Aug 13, 2022

shellscape commented Aug 13, 2022

shellscape commented Aug 13, 2022

eight04 commented Aug 13, 2022

shellscape commented Aug 13, 2022

eight04 commented Aug 13, 2022

shellscape commented Aug 13, 2022

shellscape commented Sep 10, 2022

eight04 commented Sep 11, 2022

eight04 commented Sep 11, 2022

lukastaegert Oct 14, 2022

eight04 Oct 14, 2022

eight04 commented Oct 14, 2022

lukastaegert commented Oct 14, 2022

eight04 commented Oct 14, 2022

lukastaegert left a comment

fix(inject): detect references inside object destructuring #1241

fix(inject): detect references inside object destructuring #1241

Conversation

eight04 commented Aug 12, 2022

Rollup Plugin Name: inject

Description

shellscape commented Aug 12, 2022

eight04 commented Aug 13, 2022

shellscape commented Aug 13, 2022

eight04 commented Aug 13, 2022

shellscape commented Aug 13, 2022

shellscape commented Aug 13, 2022

eight04 commented Aug 13, 2022

shellscape commented Aug 13, 2022

eight04 commented Aug 13, 2022

shellscape commented Aug 13, 2022

shellscape commented Sep 10, 2022

eight04 commented Sep 11, 2022

eight04 commented Sep 11, 2022

lukastaegert Oct 14, 2022

Choose a reason for hiding this comment

eight04 Oct 14, 2022

Choose a reason for hiding this comment

eight04 commented Oct 14, 2022

lukastaegert commented Oct 14, 2022

eight04 commented Oct 14, 2022

lukastaegert left a comment

Choose a reason for hiding this comment

Rollup Plugin Name: `inject`