Conversation
| )? | ||
| ) | | ||
| `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. |
2 participants
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.