Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants