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

Add wildcard support for passthrough #171

Merged

Conversation

lipsumar
Copy link
Contributor

This PR adds support for hostname wildcards in passThroughHostnames and interceptOnlyHostnames, as previously requested in #162.

Wildcard matching is done using URLPattern, an upcoming web standard that's perfect for this use case and may serve other purposes in mockttp. It is already implemented in many browsers and nodeJS is working on it. It is currently implemented using urlpattern-polyfill.

I changed the signature of shouldPassThrough to accept URLPattern[] instead of string[], so we don't need to keep recreating the objects. This is purely internal, the external API is unchanged.

shouldPassThrough is now unit tested. I used unit tests as it seemed fitting and some of the integration tests were already failing on main - I'm not sure why.

Let me know if you're happy with these changes, I'll then update the docs.

@lipsumar
Copy link
Contributor Author

Tests in Node 14 are failing with SyntaxError: Unexpected token '??=' - it seems that this syntax is only supported in Node 15 😬 (based on this)

Node 14 has reached end of life for some time now, how important is it to continue supporting it ?

Copy link
Member

@pimterry pimterry left a comment

Choose a reason for hiding this comment

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

This makes sense! I think I would also like to include regex support, but I'm happy to support both since it's easy to do so, and I think there are probably other places in the API where expanding to include URLPattern support would be a helpful improvement there too.

Tests in Node 14 are failing with SyntaxError: Unexpected token '??='

Node 14 obviously isn't super important since it's end of life, but it's a breaking change to drop it and I'd gently prefer not to without a strong reason. In this case, it looks like you can just use urlpattern-polyfill v8 instead and it'll work - there's very few changes since that except a minor reduction in polyfill size anyway (which is unlikely to matter in our scenarios).

some of the integration tests were already failing on main - I'm not sure why

Not sure what that means, but sounds like something that needs some investigation! They were passing in CI last week no problem, so that shouldn't really be happening. Let me know if you're still seeing this and what exactly is failing.

We should include at least one test with a wildcard in the integration tests though, just to make sure this is fully covered in the real-world scenario.

Let me know if you're happy with these changes, I'll then update the docs.

In general yes - if we can preferably avoid the breaking change to drop node 14, and we can get all the tests passing. Docs would definitely be good too, we'll want to mention this in the jsdoc for the both options.

@lipsumar
Copy link
Contributor Author

it's a breaking change to drop it and I'd gently prefer not to without a strong reason

That makes perfect sense, I understand.

Because simply importing urlpattern-polyfill won't work (that will always create a syntax error on Node 14), we can't add it as a dependency. I see 2 ways then:

  • use regexp instead of URLPattern
  • accept a predicate function, where users are free to use any logic

In both cases, I think we need to change the type of MockttpHttpsOptions['tlsPassthrough'] and MockttpHttpsOptions['tlsInterceptOnly']. They currently both accept Array<{ hostname: string }>. For regexp, it could be instead:

Array<{ hostname: string } | { hostnameRegex: RegExp }>

or

Array<{ hostname: string | RegExp }>

Same for predicate, but with (hostname: string) => boolean instead of RegExp. Or even both!

Regarding the tests, I'm happily surprised to see them all pass today as I tried to reproduce with Node 20.12 🙂 I can absolutely include wildcards in there.

Let me know which approach you prefer for MockttpHttpsOptions and I'll update the PR.

@pimterry
Copy link
Member

Because simply importing urlpattern-polyfill won't work (that will always create a syntax error on Node 14), we can't add it as a dependency.

No, we can - you can just depend on urlpattern-polyfill ^8 instead (rather than v10 - which has very few changes except dropping old node compatibility) and that will work fine.

for node 14 compatibility
Copy link

socket-security bot commented May 31, 2024

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/urlpattern-polyfill@8.0.2 None 0 103 kB sanderelias

View full report↗︎

@lipsumar
Copy link
Contributor Author

Using v8 is even simpler indeed! I did that, added integration tests for both options and documented the change. Clearly something's still wrong, and tests do pass on main. I'm investigating...

@pimterry
Copy link
Member

Oh I can explain this easy enough: the full test suite is built for both node and browser testing, and so the new test is being run as part of the browser test run too, but lots of the code inside http-combo-server won't run in browsers. As soon as you import that, lots of node-specific code & dependencies get pulled into the browser build but can't be bundled by webpack, so everything breaks (in test compilation, before the tests actually even run, I think).

(Many other tests avoid this by importing via the package root entrypoint, which uses the browser and import fields in package.json to map the root to different files, but you can't do that here unless we make this a top-level import, which would be a bit of a hack really).

Since you're actually just testing this one function, and that function will run just fine in a browser, the easiest option is to extract this into a different file that is browser-compatible (one of the utils files for example) then import just that file in the tests, and then all the tests should run fine.

@lipsumar
Copy link
Contributor Author

lipsumar commented Jun 3, 2024

That makes sense, thanks. I moved it to a new util file that has its own spec file. Looks like it's all green now! 🥳

@pimterry pimterry merged commit b916796 into httptoolkit:main Jun 3, 2024
10 checks passed
@pimterry
Copy link
Member

pimterry commented Jun 3, 2024

Nice work, thanks! 👍

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.

2 participants