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

Handle mapped exports #82

Merged
merged 5 commits into from
Mar 26, 2024
Merged

Handle mapped exports #82

merged 5 commits into from
Mar 26, 2024

Conversation

jsumners-nr
Copy link
Contributor

@jsumners-nr jsumners-nr commented Feb 5, 2024

This PR fixes an issue with mapped exports. Basically, if a module exports multiple versions of the same source then the commonjs export will end up with a file extension added. When the file extension is added, it breaks the allow list lookup and results in the hook never being applied.

Copy link

cla-checker-service bot commented Feb 5, 2024

💚 CLA has been signed

@jsumners-nr jsumners-nr marked this pull request as ready for review February 5, 2024 14:27
@jsumners-nr jsumners-nr marked this pull request as draft February 5, 2024 14:50
@jsumners-nr jsumners-nr marked this pull request as ready for review February 5, 2024 15:17
@gtback
Copy link
Member

gtback commented Feb 15, 2024

cla/check

@jsumners-nr
Copy link
Contributor Author

@gtback do you have any insight on how to get the CLA issue resolved? Our legal team says we have signed it and added myself and @bizob2828 to the CLA.

@kmudduluru
Copy link

cla/check

@jkomara
Copy link

jkomara commented Feb 23, 2024

cla/check

@jsumners-nr
Copy link
Contributor Author

🎉 one CI step complete. Now we just need to kick off the test runs.

@trentm
Copy link
Member

trentm commented Feb 23, 2024

@jsumners-nr This is on my radar, I just haven't had a chance to look yet. Sorry about the delay.

@jsumners-nr
Copy link
Contributor Author

Awesome. Thank you.

This builds on elastic#82
This first commit is a breaking test case to discuss.
@trentm
Copy link
Member

trentm commented Mar 12, 2024

@jsumners-nr Again, sorry for the delay. I was away at a work thing one of those weeks.

Thanks for this PR. I agree this should handle package entry points using "exports" (https://nodejs.org/api/packages.html#package-entry-points). I pushed a branch with a couple commits on top of yours for discussion:

The first commit adds a test case that breaks your proposed fix -- assuming I'm not missing something. Basically, I added this to the "exports" in package.json:

    "./bar": {
      "require": "./dist/cjs/bar.cjs",
      "import": "./dist/esm/bar.js"
    }

In this case, the normalization that you were proposing does not result in matching a 'mapped-exports/bar' entry in the modules array passed to the Hook.

In the second commit, I have a counter proposal that I think might work. I'd like your feedback on it.
The important part is this:
jsumners-nr/require-in-the-middle@mapped-exports...elastic:require-in-the-middle:tm-mapped-exports#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R229-R237
If the given id -- i.e. the string passed to require(...) -- matches in an entry in the Hook modules, then consider that a match. (This excludes local path requires, e.g. require('./some/local/path/...').)

@jsumners-nr
Copy link
Contributor Author

The first commit adds a test case that breaks your proposed fix

Good catch. I completely overlooked files nested in a subdirectory.

In the second commit, I have a counter proposal that I think might work. I'd like your feedback on it.

Looks good to me.

@jsumners-nr
Copy link
Contributor Author

Are we going to apply your fix to this PR?

@trentm
Copy link
Member

trentm commented Mar 19, 2024

Are we going to apply your fix to this PR?

Yes, I should get there tomorrow.

@trentm
Copy link
Member

trentm commented Mar 20, 2024

@jsumners-nr Would you like to test this with your upstream?
I'll get a release out tomorrow.

@jsumners-nr
Copy link
Contributor Author

Earliest I can do that is next week.

Copy link
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

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

/me LGTM's some of my own commits

@trentm trentm merged commit b0bd72f into elastic:main Mar 26, 2024
16 checks passed
@trentm trentm mentioned this pull request Mar 26, 2024
@jsumners-nr jsumners-nr deleted the mapped-exports branch March 27, 2024 11:39
jsumners-nr added a commit to jsumners-nr/require-in-the-middle that referenced this pull request Apr 11, 2024
PR elastic#82 originally included code to ignore the resolved file
extension. This PR restores that functionality.
jsumners-nr added a commit to jsumners-nr/require-in-the-middle that referenced this pull request Apr 11, 2024
PR elastic#82 originally included code to ignore the resolved file
extension. This PR restores that functionality.
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.

None yet

5 participants