Skip to content

fix(commonjs): register dynamic modules when a different loader (i.e typescript) loads the entry file#599

Merged
shellscape merged 1 commit intorollup:masterfrom
danielgindi:feature/ts_compat
Oct 14, 2020
Merged

fix(commonjs): register dynamic modules when a different loader (i.e typescript) loads the entry file#599
shellscape merged 1 commit intorollup:masterfrom
danielgindi:feature/ts_compat

Conversation

@danielgindi
Copy link
Contributor

@danielgindi danielgindi commented Oct 8, 2020

Rollup Plugin Name: commonjs

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

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

Description

I've encountered in a TypeScript project a weird phenomenon, where the entry file does not go through load in the commonjs plugin.
As a result, the of prepending the main entry with the registration calls of the dynamic modules - does not take place.
This means that modules were trying to call dynamic modules, but they were not in the runtime commonjs registry, and so they failed.

I fixed this by moving this phase to the transform rather than load.
A small fix that had to go hand in hand, is making sure that these commonjsRegister calls are always first as some modules may be appended to the sources array before them.

All tests pass, and this works now in typescript project.

@danielgindi
Copy link
Contributor Author

@lukastaegert Do you see any possible issues with this?
I basically just moved things around, so current tests keep passing, and a TS project can now also use this.

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little behind looking through these PRs so please be patient. So you claim that this fixes a bug. Unfortunately I am completely missing a test that was broken without this change and is fixed with this change. This would serve both as documentation and as a guarantee that future refactorings do not break this again. Could you add one?

@danielgindi
Copy link
Contributor Author

I am a little behind looking through these PRs so please be patient. So you claim that this fixes a bug. Unfortunately I am completely missing a test that was broken without this change and is fixed with this change. This would serve both as documentation and as a guarantee that future refactorings do not break this again. Could you add one?

Done. Now there's a good test for this :-)

@danielgindi
Copy link
Contributor Author

I've rebased on master. Snapshots. Every time...

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@danielgindi
Copy link
Contributor Author

@LarsDenBakker Are we merging this? It's currently a blocker for TypeScript projects!

@shellscape
Copy link
Collaborator

@danielgindi standard ops here is to wait for 2 approvals. please lend that green check mark if you think it's good to go and we'll merge it post haste

@danielgindi
Copy link
Contributor Author

@danielgindi standard ops here is to wait for 2 approvals. please lend that green check mark if you think it's good to go and we'll merge it post haste

Yeah I know :-) I intentionally tagged @LarsDenBakker. As whatever blocker it is, it's always a good idea to have another set of eyes before publishing to production...

@LarsDenBakker
Copy link
Contributor

Fine for me!

@danielgindi
Copy link
Contributor Author

Fine for me!

Yeah I think @shellscape really wants the green checkmark 🤣

@shellscape
Copy link
Collaborator

:) it's useful for tracking history

@shellscape shellscape merged commit c96f506 into rollup:master Oct 14, 2020
@danielgindi danielgindi deleted the feature/ts_compat branch October 14, 2020 16:36
@danielgindi
Copy link
Contributor Author

@shellscape I think that this fix deserves a release, as TS users currently can't fully benefit from commonjs plugin. I bumped into this and using a branch, but everybody else dont :)

@shellscape
Copy link
Collaborator

@danielgindi we merged this less than 24 hours ago. patience please! 🙏

@danielgindi
Copy link
Contributor Author

@danielgindi we merged this less than 24 hours ago. patience please! 🙏

Apologizes, I didn't mean to dictate the exact timeline 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants