plugins icon indicating copy to clipboard operation
plugins copied to clipboard

Published 20 hours ago • verified publisher icon rollup

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

Open patricklx opened this issue 1 year ago • 11 comments

Rollup Plugin Name: commonjs

This PR contains:

  • [x] bugfix
  • [ ] feature
  • [ ] refactor
  • [ ] documentation
  • [ ] other

Are tests included?

  • [ ] yes (bugfixes and features will not be merged without tests)
  • [x] no

Breaking Changes?

  • [ ] yes (breaking changes will not be merged unless absolutely necessary)
  • [x] 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.

patricklx avatar Oct 05 '23 13:10 patricklx

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?

shellscape avatar Oct 09 '23 20:10 shellscape

@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.

lukastaegert avatar Oct 10 '23 04:10 lukastaegert

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

patricklx avatar Oct 10 '23 08:10 patricklx

I also opened a PR on vitejs. But according to this comment https://github.com/vitejs/vite/pull/14569#pullrequestreview-1670210988 the functions are not bound in rollup. I also did a code search and could not find any .bind for load function. @lukastaegert

patricklx avatar Oct 11 '23 07:10 patricklx

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

patricklx avatar Oct 12 '23 07:10 patricklx

Yes, that was indeed overlooked

lukastaegert avatar Oct 12 '23 15:10 lukastaegert

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

shellscape avatar Nov 25 '23 15:11 shellscape

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

patricklx avatar Nov 27 '23 09:11 patricklx

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

shellscape avatar Jun 05 '24 02:06 shellscape

@shellscape fixed prettier

patricklx avatar Jun 20 '24 07:06 patricklx