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

fix: Incorrect exports #87

Closed
wants to merge 2 commits into from
Closed

Conversation

timfish
Copy link
Contributor

@timfish timfish commented May 29, 2024

Closes #68, Closes #77, Closes #62, Closes #60...

We need to mimic the behaviour of Node ESM resolution and my testing so far without the loader suggests that:

  • export * from only exports named exports and we don't need to pass through default exports
  • If there are duplicate named exports, none of these should be exported

Ideally we should have tests that ensure that module exports are identical both with and without the loader hook because currently this isn't the case.

To accomplish the above:

  • I've added mapExcludingDuplicates for setters which does as it suggests
  • Removed auto-renaming of default exports as this doesn't appear to be what Node does (ie. renamedExport handling incorrectly adds ES module exports #68)
  • Use the export name as the Map key so we remove duplicates
  • In the emitted code, only add exports to _ if there a corresponding key in set. Without this, the exports passed to Hook don't match the final module exports

Copy link
Contributor

Choose a reason for hiding this comment

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

The intention of this test is to verify that we can load modules like got. Can you clarify the reasoning behind removal of the removed code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's adding exports that don't exist when the loader is not in use. When testing Node without the loader, only the root default export is exposed. export * from does not include default exports.

We probably just need to add a load of troublesome libraries to devDependencies and actually test them in the tests rather than rely on synthetic test cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

We probably just need to add a load of troublesome libraries to devDependencies and actually test them in the tests rather than rely on synthetic test cases.

Agreed.

hook.js Outdated Show resolved Hide resolved
Copy link
Contributor

@jsumners-nr jsumners-nr 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 to me.

@timfish
Copy link
Contributor Author

timfish commented May 29, 2024

Closing in favour of #85 which removes more redundant code

@timfish timfish closed this May 29, 2024
@timfish timfish deleted the fix/incorrect-exports branch June 1, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants