fix(remix): Resolve Remix Request API compatibility issues. #6215
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
size-limit report 📦
|
lobsterkatie
approved these changes
Nov 16, 2022
| // chunked encoding is handled by Node.js | ||
| const search = getSearch(parsedURL); | ||
|
|
||
| // Manually spread the URL object instead of spread syntax |
There was a problem hiding this comment.
Is this your choice or just copied from their implementation? (Just curious why the spread operator is being avoided.)
Same question with the @ts-ignores below.
There was a problem hiding this comment.
Neither of the above is a blocker (just my own curiosity), so I'm going to go ahead and merge this.
There was a problem hiding this comment.
Yes, they are copied from the original implementation. Not sure what's the reason for spread operator there, but it seems they have kept the approach of node-fetch.
|
<comment deleted because I accidentally pasted the |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ref: #6139 and #6139 (comment)
This PR is not a complete solution for all issues mentioned in #6139, but it aims to solve the parsing issue, which should fix auto-instrumented usage.
Remix uses its own implementation of Fetch API on both back-end and front-end, which has different structures storing
headersandurl. That has causedRequestDataintegration to not work properly on Remix projects.I first attempted adding support to the core parser, and
PolymorphicRequest. But it seemed very complex (if possible), because we have to type Proxy objects and Symbols, which our current TypeScript version doesn't fully support.So, I vendored / modified a couple of unexported utilities from
@remix-run/web-fetch, to convert request objects to a compatible structure that we can consume inRequestDataintegration.