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

Use WHATWG URL instead of Node's built-in url module #8760

Closed
wants to merge 27 commits into from

Conversation

tylerbutler
Copy link
Member

@tylerbutler tylerbutler commented Jan 14, 2022

Summary

Partially addresses #8725.

Webpack 5, Vite, and other modern bundlers don't include Node polyfills by default, and several of the functions (parse, resolve) have been deprecated since Node 11 in favor of the WHATWG URL spec.

This PR removes our use of the url module, replacing it with an isomorphic implementation.

Updates

2022-02-03: The whatwg-url package worked almost everywhere. The exception was the end-to-end-tests project, which needed support for relative URLs without a base, so there I used the iso-url package. This only affects the test project.

2022-01-31: The browser URL implementation has a bug, it seems. See an example in the playground here.

let fluidScheme = new URL("fluid-test://localhost:3000/tenantId/1643676316659");
let https = new URL("https://localhost:3000/tenantId/1643676316659");
console.log(fluidScheme.pathname === https.pathname);

This code should print true, but it's false in the latest Edge and Firefox. fluidScheme.pathname is returning "//localhost:3000/tenantId/1643676316659". It should return "/tenantId/1643676316659".

Anyway, I've switched to whatwg-url and am going to use that everywhere. I trust it more.

2022-01-18: Testing the WHATWG URL API -- I'm unclear if it will "just work" across all the build targets we care about. But all local testing passes.

2022-01-14: Debugging test failures.


@github-actions github-actions bot added area: driver Driver related issues area: loader Loader related issues labels Jan 14, 2022
@github-actions github-actions bot added the area: tests Tests to add, test infrastructure improvements, etc label Jan 15, 2022
@tylerbutler tylerbutler marked this pull request as draft January 15, 2022 03:41
@github-actions github-actions bot requested review from vladsud and jatgarg January 19, 2022 00:57
@github-actions github-actions bot removed the area: tests Tests to add, test infrastructure improvements, etc label Jan 19, 2022
const parsedUrl = parse(resolvedUrl.url);
const [, tenantId, documentId] = parsedUrl.path ? parsedUrl.path.split("/") : [];
const parsedUrl = new URL(resolvedUrl.url);
const [, tenantId, documentId] = parsedUrl.pathname ? parsedUrl.pathname.split("/") : [];
Copy link
Member Author

@tylerbutler tylerbutler Jan 19, 2022

Choose a reason for hiding this comment

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

~url.parse().path is not the same as new URL().pathname. The former includes the query string, while the latter does not. This code change is therefor a behavior change, but I don't think it matters because the query isn't parsed. Moreover, similar functions exist elsewhere in our codebase, and in those cases, the existing code already used .pathname.

Wrong.

@tylerbutler tylerbutler changed the title Use url-parse instead of Node's built-in url module Use WHATWG URL instead of Node's built-in url module Jan 19, 2022
@github-actions github-actions bot removed the area: driver Driver related issues label Jan 20, 2022
@github-actions github-actions bot added area: examples Changes that focus on our examples area: tests Tests to add, test infrastructure improvements, etc labels Feb 2, 2022
@tylerbutler tylerbutler marked this pull request as ready for review February 3, 2022 18:00
@tylerbutler tylerbutler requested review from a team as code owners February 3, 2022 18:00
@@ -16,7 +16,7 @@ module.exports = {
"/**/dist/test/*.spec.js"
],

testEnvironment: "jsdom",
testEnvironment: "node",
Copy link
Member Author

Choose a reason for hiding this comment

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

This was needed to get the tests passing, but I don't understand why or if this is really a safe change.

@tylerbutler
Copy link
Member Author

tylerbutler commented Mar 10, 2022

  1. As-is, this PR increases bundle size by a lot. 240kb for the loader. For this reason, the PR won't be merged.
  2. I used the whatwg-url package because the browser implementations have a "bug" that causes them to parse our test URLs in an unexpected way. However, @ChumpChief points out that we're using a non-standard (read: invalid) protocol, fluid-test, and thus the parsing behavior of the browser is not incorrect, even though it's not what I expected. To quote Matt, "Parsers must parse any scheme including custom ones, but the rules of how to break down all the content after the scheme differs. If it's a known "special" scheme, then it gets further broken down into the components we're used to in http urls. if it's a custom scheme, then the parser should not presume to know anything about the remainder of the url, and basically shove all the remainder into 'path'". However, reasonable people disagree that this is correct -- hence the whatwg-url library parses our URLs "correctly". See this issue for some more context: different results in parsing hostname / origin with custom uri schemes jsdom/whatwg-url#110 (comment)
  3. Given (1) and (2), the fastest way to address the Webpack 5/Vite/etc. compat issues is to manually depend on the node polyfill, which is this package: https://github.com/defunctzombie/node-url That should have no practical bundle size impact (negative nor positive), but doesn't seem like the "right fix" long-term. It's just shoring up dependence on a now non-standard parsing behavior.
  4. The more correct fix is to remove our dependence on parsing non-standard URLs. @ChumpChief investigated that and filed Stop using non-standard protocols in URLs and switch to native URL() over url-parse #9236 to track.

Given the complexity of (4), I'm inclined to do (3) as a short-term solution. Regardless that will be in a separate PR.

@tylerbutler tylerbutler deleted the isomorphic branch February 1, 2023 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: dds: propertydds area: driver Driver related issues area: examples Changes that focus on our examples area: loader Loader related issues area: tests Tests to add, test infrastructure improvements, etc dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants