Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support RollupJS reexports property checks #41

Merged
merged 3 commits into from
Mar 8, 2021

Conversation

guybedford
Copy link
Collaborator

Fixes #38.

This extends the p !== 'default' pattern to add the !exports.hasOwnProperty(p) reexports check as well. I've also included a check for Object.hasOwnProperty.call(exports, p) for the case if RollupJS ever decides to use this form as well.

//cc @nicolo-ribaudo for review. I've also refactored the Babel hasOwnProperty check here making the prototype optional.

test/_unit.js Outdated Show resolved Hide resolved
if (ch !== 33/*!*/) break;
pos += 1;
ch = commentWhitespace();
if (source.startsWith(id, pos)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Which tool uses exports.hasOwnProperty?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

RollupJS just does this now as per #38. I'm not sure if there's anything for the collision case, that's why I added the Object.hasOwnProperty as well in case that gets added in future //cc @lukastaegert.

@guybedford guybedford merged commit 9fa5feb into master Mar 8, 2021
@guybedford guybedford deleted the rollup-exports-prop-check branch March 8, 2021 08:50
});

Object.keys(external6).forEach(function (k) {
if (k !== 'default' && !external6.hasOwnProperty(k)) exports[k] = external6[k];

Choose a reason for hiding this comment

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

It's exports.hasOwnProperty not external6.hasOwnProperty 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for spotting this, wish we'd got this on the first review! Posted #59.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sodatea I've released the update here in 1.2.2 (a96e7b5) and am starting the process to backport the updates across all Node.js release lines for the releases next month. If you are able to verify the fix separately that would be a huge help to ensure there are no further regressions here.

Choose a reason for hiding this comment

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

Thanks! I think it works correctly now.

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.

reexports detection incompatible with output from Rollup@^2.39.0
3 participants