elastic / require-in-the-middle

Module to hook into the Node.js require function
MIT License
168 stars 26 forks source link

Handle mapped exports #82

Closed jsumners-nr closed 7 months ago

jsumners-nr commented 9 months ago

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[bot] commented 9 months ago

💚 CLA has been signed

gtback commented 9 months ago

cla/check

jsumners-nr commented 9 months ago

@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 commented 8 months ago

cla/check

jkomara commented 8 months ago

cla/check

jsumners-nr commented 8 months ago

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

trentm commented 8 months ago

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

jsumners-nr commented 8 months ago

Awesome. Thank you.

trentm commented 8 months ago

@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: https://github.com/jsumners-nr/require-in-the-middle/compare/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 commented 8 months ago

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 commented 7 months ago

Are we going to apply your fix to this PR?

trentm commented 7 months ago

Are we going to apply your fix to this PR?

Yes, I should get there tomorrow.

trentm commented 7 months ago

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

jsumners-nr commented 7 months ago

Earliest I can do that is next week.