Skip to content

esm: use undici/fetch data url parser - #54748

Merged
aduh95 merged 8 commits into
nodejs:mainfrom
KhafraDev:data-url-fetch-parser
Sep 7, 2024
Merged

esm: use undici/fetch data url parser#54748
aduh95 merged 8 commits into
nodejs:mainfrom
KhafraDev:data-url-fetch-parser

Conversation

@KhafraDev

@KhafraDevKhafraDev commented Sep 3, 2024

Copy link
Copy Markdown
Member

Using the fetch parser, rather than a regex, should fix most of these edge cases.

It would probably be better to export the parser directly so we can use it for the sync parsing too. Just wanted to gauge how correct or welcome this change would be.

fetch should also be used for blob: urls, whenever support is added for them, as it handles edge cases regarding those as well. If someone ever brings back http/https imports, fetch should probably be used there as well. :)

Fixes#53775
Fixes#42890
Closes#51324

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-botnodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Sep 3, 2024
Comment threadtest/es-module/test-esm-data-urls.js Outdated
} catch (e) {
assert.strictEqual(e.code, 'ERR_INVALID_URL');
}
await assert.rejects(import(plainESMURL), { code: 'ERR_UNKNOWN_MODULE_FORMAT' })

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

data:invalid,null is a valid data url afaik

@aduh95

Copy link
Copy Markdown
Contributor

One of the problem of this approach is that Undici is not at all written in a way to be robust against prototype mutation, which seems to be a deal breaker when it comes to loading modules.

@KhafraDev

Copy link
Copy Markdown
MemberAuthor

that's true, maybe I could rip the data url parser from undici and implement it here? Could be useful for the node:util or something as well.

@avivkelleravivkeller added fetch Issues and PRs related to the Fetch API and removed errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Sep 3, 2024
@KhafraDev

KhafraDev commented Sep 4, 2024

Copy link
Copy Markdown
MemberAuthor

I took undici's data url parser and added primordials, it's entirely possible I missed things or misused them. It's also likely possible to replace the mimetype parsing with the built-in one.

@mcollina

Copy link
Copy Markdown
Member

@KhafraDev can you also port the tests for the tests for the data url parser?

@ljharbljharb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

feel free to ignore these, but they might make things simpler and faster

Comment threadlib/internal/data_url.js Outdated
Comment threadlib/internal/data_url.js
Comment threadlib/internal/data_url.js Outdated
Comment threadlib/internal/data_url.js Outdated
Comment threadlib/internal/data_url.js Outdated
Comment threadlib/internal/data_url.js Outdated
Comment threadlib/internal/data_url.js
Comment threadlib/internal/data_url.js
Comment threadlib/internal/data_url.js
@KhafraDev
KhafraDev marked this pull request as ready for review September 4, 2024 16:41
@KhafraDev

Copy link
Copy Markdown
MemberAuthor

I took the WPTs for fetching data urls, if there's a more complete dataset, or if there's one specifically for imports, let me know.

Comment threadlib/internal/data_url.js Outdated
Comment threadtest/parallel/test-data-url.js Outdated
Comment threadtest/parallel/test-data-url.js Outdated

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@codecov

codecovBot commented Sep 4, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 91.16022% with 32 lines in your changes missing coverage. Please review.

Project coverage is 87.61%. Comparing base (5949e16) to head (86d6382).
Report is 344 commits behind head on main.

Files with missing linesPatch %Lines
lib/internal/data_url.js91.76%26 Missing and 3 partials ⚠️
lib/internal/modules/esm/load.js70.00%3 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #54748 +/- ##
========================================
Coverage 87.60% 87.61% ========================================
Files 650 651 +1 Lines 182829 183289 +460 Branches 35379 35434 +55 ========================================
+ Hits 160173 160591 +418 - Misses 15928 15950 +22 - Partials 6728 6748 +20 
Files with missing linesCoverage Δ
lib/internal/modules/esm/load.js92.54% <70.00%> (-0.42%)⬇️
lib/internal/data_url.js91.76% <91.76%> (ø)

... and 34 files with indirect coverage changes

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@KhafraDev

This comment was marked as off-topic.

@KhafraDev

This comment was marked as off-topic.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@KhafraDev

This comment was marked as off-topic.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@aduh95

This comment was marked as off-topic.

@KhafraDev

This comment was marked as off-topic.

@nodejs-github-bot

nodejs-github-bot commented Sep 6, 2024

Copy link
Copy Markdown
Collaborator

@aduh95
aduh95 merged commit 6c85d40 into nodejs:mainSep 7, 2024
@aduh95

Copy link
Copy Markdown
Contributor

Landed in 6c85d40

aduh95 pushed a commit that referenced this pull request Sep 12, 2024
Fixes: #53775
PR-URL: #54748
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request Sep 16, 2024
@KhafraDev
KhafraDev deleted the data-url-fetch-parser branch September 19, 2024 03:18
@targos

targos commented Sep 30, 2024

Copy link
Copy Markdown
Member

I tried to include this in v20.x, but it broke HTTP imports (which were removed from main/v22.x)

See https://github.com/nodejs/node/actions/runs/11102823645/job/30843465228?pr=55170

@KhafraDev

Copy link
Copy Markdown
MemberAuthor

The test that failed is:

it('data: URL can always import other data:',async()=>{constdata=newURL('data:text/javascript,');data.searchParams.set('body','import \'data:text/javascript,import \'data:\'');// doesn't throwconstempty=awaitimport(data.href);assert.ok(empty);});

Where empty is 'data:text/javascript,?body=import+%27data%3Atext%2Fjavascript%2Cimport+%27data%3A%27'. The search gets parsed as part of the body (as data: urls cannot have search params). The test is faulty, there were some I changed in this PR that were similarly incorrect.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.esmIssues and PRs related to the ECMAScript Modules implementation.fetchIssues and PRs related to the Fetch APIneeds-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Data URLs truncated esm module import with data scheme has bug with ternary operator in object

10 participants

@KhafraDev@nodejs-github-bot@aduh95@mcollina@targos@ljharb@jasnell@anonrig@avivkeller@tsctx