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

implement domainToASCII and domainToUnicode #2629

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Aug 29, 2024

Implements domainToASCII and domainToUnicode and exports them under node:url where URL and URLSearchParams also exist.

@anonrig anonrig requested review from jasnell and npaun August 29, 2024 20:45
@anonrig anonrig requested review from a team as code owners August 29, 2024 20:45
@anonrig anonrig force-pushed the yagiz/add-url-implementations branch from c56f8ff to f4dea61 Compare August 29, 2024 20:46
src/node/url.ts Show resolved Hide resolved
@anonrig anonrig force-pushed the yagiz/add-url-implementations branch from f4dea61 to 7035c6c Compare August 29, 2024 20:49
src/workerd/api/node/url.c++ Outdated Show resolved Hide resolved
src/workerd/api/node/url.c++ Outdated Show resolved Hide resolved
@anonrig anonrig force-pushed the yagiz/add-url-implementations branch from 7035c6c to 59c79e6 Compare August 29, 2024 21:00
@jasnell jasnell requested review from IgorMinar and vicb August 29, 2024 21:01
src/node/url.ts Outdated Show resolved Hide resolved
@anonrig anonrig force-pushed the yagiz/add-url-implementations branch 2 times, most recently from 00b9b2e to 6daf248 Compare August 29, 2024 21:03
src/node/url.ts Outdated Show resolved Hide resolved
src/node/url.ts Outdated Show resolved Hide resolved
src/node/url.ts Outdated Show resolved Hide resolved
src/node/url.ts Outdated Show resolved Hide resolved
@anonrig anonrig force-pushed the yagiz/add-url-implementations branch 2 times, most recently from aaa3ae0 to 86803b0 Compare August 29, 2024 21:07
src/workerd/api/node/url.c++ Show resolved Hide resolved
src/workerd/api/node/url.c++ Outdated Show resolved Hide resolved
src/workerd/api/node/url.c++ Outdated Show resolved Hide resolved
@anonrig anonrig force-pushed the yagiz/add-url-implementations branch 2 times, most recently from 64db339 to 5769495 Compare August 29, 2024 21:36
@anonrig anonrig requested review from npaun and jasnell August 29, 2024 21:39
@anonrig anonrig force-pushed the yagiz/add-url-implementations branch from 5769495 to 2304cbb Compare August 29, 2024 21:41
@jasnell
Copy link
Member

jasnell commented Aug 29, 2024

@IgorMinar ... please take a look

Copy link
Collaborator

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

LGTM

For those wondering how this PR relates to unjs/unenv#299, @anonrig clarified with me that these two methods are nearly impossible to implement in userland in a performant way. So we are implementing them natively, while other parts of node:url could come in via unenv.

// fyi: @pi0

@jasnell
Copy link
Member

jasnell commented Aug 29, 2024

@pi0 ... so you know what to expect... these will be exported by the runtime as the node:url module. The bundler will essentially need to alias node:url, use the process.getBuiltinModule('node:url') API to get the built-in version to extend (that has these methods), then re-export the fully patched module.

@IgorMinar
Copy link
Collaborator

@pi0 ... so you know what to expect... these will be exported by the runtime as the node:url module. The bundler will essentially need to alias node:url, use the process.getBuiltinModule('node:url') API to get the built-in version to extend (that has these methods), then re-export the fully patched module.

Yes! except unenv will do exactly this in a new file we'll create under https://github.com/unjs/unenv/tree/main/src/runtime/node/url called $cloudflare.ts, following the existing precedence for hybrid modules: https://github.com/unjs/unenv/blob/main/src/runtime/node/buffer/%24cloudflare.ts

Bundlers then pick up url/$cloudflare.ts as instructed by the cloudflare preset (which also needs to be updated to include url) and combine it all into the final bundle.

@jasnell
Copy link
Member

jasnell commented Aug 29, 2024

@anonrig ... as discussed in chat, let's make sure this is added to the test:

diff --git a/src/workerd/api/node/tests/url-nodejs-test.js b/src/workerd/api/node/tests/url-nodejs-test.js
index ab683c9f..c75fbfad 100644
--- a/src/workerd/api/node/tests/url-nodejs-test.js
+++ b/src/workerd/api/node/tests/url-nodejs-test.js
@@ -77,3 +77,11 @@ export const urlAndSearchParams = {
     );
   },
 };
+
+export const getBuiltinModule = {
+  async test() {
+    const bim = process.getBuiltinModule('node:url');
+    const url = await import('node:url');
+    strictEqual(bim, url.default);
+  }
+};
diff --git a/src/workerd/api/node/tests/url-nodejs-test.wd-test b/src/workerd/api/node/tests/url-nodejs-test.wd-test
index 1be17b89..92589f61 100644
--- a/src/workerd/api/node/tests/url-nodejs-test.wd-test
+++ b/src/workerd/api/node/tests/url-nodejs-test.wd-test
@@ -8,7 +8,7 @@ const unitTests :Workerd.Config = (
           (name = "worker", esModule = embed "url-nodejs-test.js")
         ],
         compatibilityDate = "2023-10-01",
-        compatibilityFlags = ["nodejs_compat"],
+        compatibilityFlags = ["nodejs_compat_v2"],
       )
     ),
   ],

@anonrig anonrig force-pushed the yagiz/add-url-implementations branch from 2304cbb to 1501ac0 Compare August 29, 2024 23:44
@anonrig anonrig merged commit eeb5f84 into main Aug 30, 2024
12 of 13 checks passed
@anonrig anonrig deleted the yagiz/add-url-implementations branch August 30, 2024 00:44
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.

4 participants