Skip to content

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.

@cla-checker-service
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

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
Contributor

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.

trentm added 2 commits March 11, 2024 17:33
This builds on nodejs#82
This first commit is a breaking test case to discuss.
@trentm
Copy link
Contributor

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
Contributor

trentm commented Mar 19, 2024

Are we going to apply your fix to this PR?

Yes, I should get there tomorrow.

@trentm
Copy link
Contributor

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
Contributor

@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 nodejs:main Mar 26, 2024
@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 nodejs#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 nodejs#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.

5 participants