fix(commonjs): keep `this` context for callbacks. #1603

patricklx · 2023-10-05T13:12:26Z

Rollup Plugin Name: `commonjs`

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:

Description

it's not possible to use the commonjs plugin in vitejs, as some callback functions are not called with the original context.
edit: As I understand now, rollup already binds the context. but vitejs doesn't.

lukastaegert · 2023-10-05T13:28:58Z

packages/commonjs/src/resolve-require-sources.js

@@ -108,7 +108,7 @@ export function getRequireResolver(extensions, detectCyclesAndConditional, curre
        setInitialParentType(parentId, parentMeta.initialCommonJSType);
        await Promise.all(
          parentRequires.map(({ resolved, isConditional }) =>
-            analyzeRequiredModule(parentId, resolved, isConditional, this.load)
+            analyzeRequiredModule(parentId, resolved, isConditional, this.load.bind(this))


Fair enough, but Rollup already this-binds all context functions. This is by design, but admittedly, this is not well-communicated.

ah, okay. this issue appeared while using commonjs plugin with vitejs.

Interesting. So I assume this happened in dev mode? Then this would mean that Vite does not this.bind. Maybe an issue to raise with Vite. But this would also make it harder to write a test as you would need to test with a fake plugin context.

Also, Andrew will not like that you skipped the PR template where you should have added all this information.

I think so too ^^ 😅

I added it now

shellscape · 2023-10-09T20:01:22Z

Looks like you need an upstream update:

git remote add upstream [email protected]:rollup/plugins.git
git fetch upstream
git merge upstream/master
git push

That should get you set so that actions run without errors.

We'll also need a new test that tests this explicit change before we can merge. It's the only way we'll be able to protect against a regression.

@lukastaegert what do you think about this change on the whole, given the info in the other comment thread?

lukastaegert · 2023-10-10T04:41:54Z

@shellscape I think it is ok.
It would be nice to have some kind of test, but this is really difficult as you would basically need to write a mock Rollup context, which in itself would be hard to maintain and likely not worth the effort, so we can go through with this once CI passes.

patricklx · 2023-10-10T08:57:34Z

I think now this should be resolved in vitejs to also bind the functions just like rollup.

patricklx · 2023-10-11T07:31:08Z

I also opened a PR on vitejs.
But according to this comment vitejs/vite#14569 (review)
the functions are not bound in rollup.
I also did a code search and could not find any .bind for load function.
@lukastaegert

patricklx · 2023-10-12T07:10:45Z

By the way, addWatchFile is not bound and access this.
https://github.com/rollup/rollup/blob/fac5f1c1c12dc0409ff090664e305586747dc2f7/src/utils/PluginContext.ts#L61

lukastaegert · 2023-10-12T15:01:28Z

Yes, that was indeed overlooked

shellscape · 2023-11-25T15:03:22Z

@patricklx what's the state of this PR at the moment?

patricklx · 2023-11-27T09:23:48Z

it should be ready. I also making a PR to vite as well

shellscape · 2024-06-05T02:05:51Z

@patricklx circling back to this one. looks like linting need to be run locally, prettier isn't happy in CI.

patricklx · 2024-06-20T07:47:37Z

@shellscape fixed prettier

shellscape · 2024-09-23T16:32:14Z

@patricklx thanks for fixing the last issue. it looks like your changes are good to go, but we require tests for fixes and features to protect against regressions. would you be able to add a test for these changes?

shellscape · 2024-12-15T16:21:42Z

Closing as abandoned.

keep this context for callbacks

93387d2

patricklx requested a review from shellscape as a code owner October 5, 2023 13:12

patricklx changed the title ~~keep this context for callbacks~~ [commonjs] keep this context for callbacks Oct 5, 2023

tada5hi changed the title ~~[commonjs] keep this context for callbacks~~ fix(commonjs): keep this context for callbacks Oct 5, 2023

lukastaegert reviewed Oct 5, 2023

View reviewed changes

patricklx mentioned this pull request Oct 10, 2023

fix(dev): bind plugin context functions vitejs/vite#14569

Merged

9 tasks

Merge branch 'rollup:master' into patch-1

f5cfc4d

shellscape force-pushed the master branch 7 times, most recently from a69c299 to a9b9cd3 Compare October 25, 2023 21:01

missing parenthesis

2d31405

bluwy mentioned this pull request Jan 16, 2024

perf: do not bind plugin hook context functions vitejs/vite#15610

Merged

4 tasks

shellscape changed the title ~~fix(commonjs): keep this context for callbacks~~ fix(commonjs): keep this context for callbacks Jun 5, 2024

fix prettier

b70ad2a

shellscape changed the title ~~fix(commonjs): keep this context for callbacks~~ fix(commonjs): keep this context for callbacks. Sep 23, 2024

shellscape closed this Dec 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(commonjs): keep `this` context for callbacks. #1603

fix(commonjs): keep `this` context for callbacks. #1603

patricklx commented Oct 5, 2023 •

edited

Loading

lukastaegert Oct 5, 2023

patricklx Oct 5, 2023 •

edited

Loading

lukastaegert Oct 5, 2023

lukastaegert Oct 5, 2023

tada5hi Oct 5, 2023

patricklx Oct 5, 2023

shellscape commented Oct 9, 2023

lukastaegert commented Oct 10, 2023

patricklx commented Oct 10, 2023

patricklx commented Oct 11, 2023 •

edited

Loading

patricklx commented Oct 12, 2023

lukastaegert commented Oct 12, 2023

shellscape commented Nov 25, 2023

patricklx commented Nov 27, 2023

shellscape commented Jun 5, 2024

patricklx commented Jun 20, 2024

shellscape commented Sep 23, 2024

shellscape commented Dec 15, 2024

fix(commonjs): keep this context for callbacks. #1603

fix(commonjs): keep this context for callbacks. #1603

Conversation

patricklx commented Oct 5, 2023 • edited Loading

Rollup Plugin Name: commonjs

Description

lukastaegert Oct 5, 2023

Choose a reason for hiding this comment

patricklx Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

lukastaegert Oct 5, 2023

Choose a reason for hiding this comment

lukastaegert Oct 5, 2023

Choose a reason for hiding this comment

tada5hi Oct 5, 2023

Choose a reason for hiding this comment

patricklx Oct 5, 2023

Choose a reason for hiding this comment

shellscape commented Oct 9, 2023

lukastaegert commented Oct 10, 2023

patricklx commented Oct 10, 2023

patricklx commented Oct 11, 2023 • edited Loading

patricklx commented Oct 12, 2023

lukastaegert commented Oct 12, 2023

shellscape commented Nov 25, 2023

patricklx commented Nov 27, 2023

shellscape commented Jun 5, 2024

patricklx commented Jun 20, 2024

shellscape commented Sep 23, 2024

shellscape commented Dec 15, 2024

fix(commonjs): keep `this` context for callbacks. #1603

fix(commonjs): keep `this` context for callbacks. #1603

patricklx commented Oct 5, 2023 •

edited

Loading

Rollup Plugin Name: `commonjs`

patricklx Oct 5, 2023 •

edited

Loading

patricklx commented Oct 11, 2023 •

edited

Loading