fix(dev): bind plugin context functions

[patricklx]

rollup binds the plugin context functions to itself and plugins use that.
e.g. commonjs.

closes rollup/plugins#1603

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the PR Title Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[sapphi-red]

Would you add a test to https://github.com/vitejs/vite/blob/main/packages/vite/src/node/server/__tests__/pluginContainer.spec.ts?

[patricklx]

@sapphi-red this is ready

[sapphi-red]

Thanks!

[bluwy]

I guess there's no harm to bind this, but Rollup doesn't call bind(this) on the plugin context either: https://github.com/rollup/rollup/blob/fac5f1c1c12dc0409ff090664e305586747dc2f7/src/utils/PluginContext.ts#L60-L114

It happens that the commonjs plugin assumes that this.load isn't binding sensitive and it worked. So having this fixed on the commonjs plugin side would also be nice.

[patricklx]

I looked at the functions again and i noticed that the concept is to have all functions either with bind or not binding sensitive.

[lukastaegert]

but Rollup doesn't call bind(this) on the plugin context either

Of course it does not bind. You do not need to bind if you do not access this.

[lukastaegert]

Ok fair enough, this.addWatchFile indeed breaks that assumption...

[bluwy]

@lukastaegert I don't mind to proceed with this PR if you intended for the plugin context to already be bound. Would you prefer we go with this then?

I was thinking it's odd that you'd pass this.load around and expect the this context to still maintain. Usually handling references like this need to be coded defensively with a this.load.bind(this). And we only found about this issue after years.

If we do go with this PR, it's not completed yet. There's still functions not bound like this.getModuleIds, this.getModuleInfo, this.emitFile etc (basically all just in case we ever use this). After that we could merge it.

[lukastaegert]

By the way, the this reference in this.addWatchFile has been removed now.

[patak-dev]

I would prefer these functions to not be bound, but given that this was already an expectation in the Rollup ecosystem, I think we could merge this PR. Later on, we could still decide to remove this feature, but do it in sync both in Rollup and Vite if there is consensus and it is justified.

[lukastaegert]

but do it in sync both in Rollup

Well, Rollup does not bind, it just does not use this 😉

[patak-dev]

but do it in sync both in Rollup

Well, Rollup does not bind, it just does not use this 😉

Ha, yes, we are on the same page, but just in case for reference I mean if it is ever decided that users can't expect context functions to work without being bound to its context (no matter how it is implemented), it would be good if it is a change in both Rollup and Vite dev.

By the way, all methods in the Vite dev server can also be used without being bound to the vite server, but this is an undocumented implementation detail. I hope no one is relying on this right now, maybe we'll end up having issues in the future if we ever change to a different implementation.