Fix #337 Keep isCjsPromises between commonjs() calls #338
Conversation
| @@ -37,6 +37,7 @@ function startsWith ( str, prefix ) { | |||
| return str.slice( 0, prefix.length ) === prefix; | |||
| } | |||
|
|
|||
| const isCjsPromises = Object.create(null); | |||
There was a problem hiding this comment.
My worry is this could be cache poisoning if this changes between runs (eg a file changes from ESM to CJS or vice versa).
Do you know what the root cause was on the stalling that this fixes it?
There was a problem hiding this comment.
I think it's because rollup won't call transform again for paths where code hasn't changed, so .resolve will never be called. I'm unfortunately not familiar with the flow, but yes, I'm sure there's a better way.
There was a problem hiding this comment.
Ah thanks that makes complete sense to understand the cause.
|
The new cache work happening in core will actually resolve this issue. So I would suggest we merge this even though it isn't quite correct, as it will be able to be corrected soon enough (once rollup/rollup#2389 lands). |
|
@guybedford doesn't look this has been published to npm yet, can you do so? |
|
Sure, unfortunately I don't have publishing rights to this one. //cc @lukastaegert |
|
@guybedford I just invited you. |
|
published |
|
Thanks for the quick turnaround! |
When using rollup cache, rollup won't call transform if it has a cache, so it appears the promise will never resolve because isCjsPromises is cleared between calls to commonjs().
I had to change the tests overall a bit to ensure they ran as if commonjs was loaded anew every time. I'm not sure it's the best way to do it though, but since there wasn't any state saved between runs until now it should be ok.