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: explicitly import http and https modules due to bundler limitations #2586

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

perrin4869
Copy link

My CI broke because of a change in #2572
Some other people later mentioned this issue, but no one has yet found the root cause
Honestly I am curious but don't have the time to dig into it at this moment...

Copy link
Member

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

Ok, weird. Maybe some security issue?

@perrin4869
Copy link
Author

Ohhh ok I finally got it 😅
It's because of the bundler 😅

Could not dynamically require \"http\". Please configure the dynamicRequireTargets or/and ignoreDynamicRequires option of @rollup/plugin-commonjs appropriately for this require call to work.

I'll try to come up with a workaround, but yeah, it's not necessarily a bug with nock after-all. I guess it was coded in this way originally in order to play ball with rollup, maybe the author was using a setup similar to mine?

@perrin4869 perrin4869 closed this Feb 8, 2024
@gr2m
Copy link
Member

gr2m commented Feb 8, 2024

It's usually a problem with bundling. I'd reopen and merge this PR but add a comment that we need to explicitly import both modules due to some bundlers not being able to handle dynamic imports

@gr2m gr2m reopened this Feb 8, 2024
lib/common.js Outdated Show resolved Hide resolved
@gr2m gr2m changed the title fix: for some reason you need to require both http and https fix: explicitly import http and https modules due to bundler limitations Feb 8, 2024
@gr2m gr2m enabled auto-merge (squash) February 8, 2024 16:03
@perrin4869
Copy link
Author

hm... yeah, avoiding the dynamic imports would make life easier for the bundlers, but this should also be fixed in rollup side ^^;;
I'm leaning towards sending a PR to allow for dynamic require of built-in node modules, that should fix the issue here at least

@gr2m
Copy link
Member

gr2m commented Feb 8, 2024

Not all bundlers use rollup, we can't rely on it. This might become a non-issue anyway in future versions as we change the way requests are intercepted, I'd just revert to the previous behavior which was importing both modules statically

@gr2m
Copy link
Member

gr2m commented Feb 8, 2024

Not sure why CI is failing though 😓

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

3 participants