fix: RollupJS reexports detection #59
Conversation
| @@ -113,7 +113,7 @@ EXPORT_STAR_LIB: `Object.keys(` IDENTIFIER$1 `).forEach(function (` IDENTIFIER$2 | |||
| (`if (` IDENTIFIER$2 `in` EXPORTS_IDENTIFIER `&&` EXPORTS_IDENTIFIER `[` IDENTIFIER$2 `] ===` IDENTIFIER$1 `[` IDENTIFIER$2 `]) return` `;`)? | |||
| )? | |||
| ) | | |||
| `if (` IDENTIFIER$2 `!==` ( `'default'` | `"default"` ) (`&& !` (`Object` `.prototype`? `.hasOwnProperty.call(` IDENTIFIER$1 `, ` IDENTIFIER$2 `)` | IDENTIFIER$1 `.hasOwnProperty(` IDENTIFIER$2 `)`))? `)` | |||
| `if (` IDENTIFIER$2 `!==` ( `'default'` | `"default"` ) (`&& !` (`Object` `.prototype`? `.hasOwnProperty.call(` IDENTIFIER `, ` IDENTIFIER$2 `)` | IDENTIFIER `.hasOwnProperty(` IDENTIFIER$2 `)`))? `)` | |||
There was a problem hiding this comment.
I think it should be like this:
| `if (` IDENTIFIER$2 `!==` ( `'default'` | `"default"` ) (`&& !` (`Object` `.prototype`? `.hasOwnProperty.call(` IDENTIFIER `, ` IDENTIFIER$2 `)` | IDENTIFIER `.hasOwnProperty(` IDENTIFIER$2 `)`))? `)` | |
| `if (` IDENTIFIER$2 `!==` ( `'default'` | `"default"` ) (`&& !` (`Object` `.prototype`? `.hasOwnProperty.call(` EXPORTS_IDENTIFIER `, ` IDENTIFIER$2 `)` | EXPORTS_IDENTIFIER `.hasOwnProperty(` IDENTIFIER$2 `)`))? `)` |
There was a problem hiding this comment.
Or well, maybe not because EXPORTS_IDENTIFIER also allows module.exports. But we should expect the exact exports identifier, not a generic one.
There was a problem hiding this comment.
The reason this is generic is due to the export names object check for Babel here - https://github.com/guybedford/cjs-module-lexer/blob/master/test/_unit.js#L171.
| if (ch === 79/*O*/ && source.startsWith('bject', pos + 1) && source[pos + 6] === '.') { | ||
| if (!tryParseObjectHasOwnProperty(it_id)) break; | ||
| } |
There was a problem hiding this comment.
This has just been moved here from the old else branch without changing the semantics, right?
There was a problem hiding this comment.
Yes, the order switch is so that pos doesn't get changed for the fallback check.
|
I'm seriously considering if this PR is even worth merging given the current state of compatibility over older Node.js versions. There is a very real risk of package interops working for some Node.js versions and not others which seems very risky. Does anyone have any feedback on this? |
|
Isn't it already the case? Or did you backport the last cjs lexer version to every Node.js version using it? |
|
@nicolo-ribaudo up until now I've been running backports back to 12 even, since these are all backwards compatible. The issue is more just something working only on newer 12 / 14 which is the urgency to stabilize here. |
This fixes the RollupJS reexports detection implemented in #41 due to the typo pointed out by @sodatea in https://github.com/guybedford/cjs-module-lexer/pull/41/files#r667619960.
@nicolo-ribaudo review would be very welcome if you have the time, cannot afford any mistakes here if we do a full backport.