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

fix: ban import assertions during publishing #427

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

lucacasonato
Copy link
Member

@lucacasonato lucacasonato commented Apr 22, 2024

Only import attributes are allowed now.

Technically this also bans import "./deno.json" /* assert */ with { type: "json" }; but hey, who cares?! :D

Fixes #418

Only import attributes are allowed now.
@lucacasonato lucacasonato requested a review from dsherret April 22, 2024 11:13
@marvinhagemeister
Copy link
Contributor

What about dynamic assertions like:

await import('./foo.json', { assert: { type: 'json' }});

Looking at the PR I think these are still allowed.

@lucacasonato
Copy link
Member Author

lucacasonato commented Apr 23, 2024

It is a lot of effort to implement this for dynamic import, and considering that users already need to assume that dynamic import may fail, and also we can not analyze all cases (for example user passes the options bag via a binding).

Also if we did ban this, there would be no way to write code that works both with systems that do and do not understand attributes yet (for example await import("./data.json", { assert: { type: "json" }, with: { type: "json" }) works in a larger number of Node versions than just with: { type: "json" }.

@marvinhagemeister
Copy link
Contributor

Makes sense, didn't know that the dynamic variant shared wider support in Node. In that case I'm fine with landing this.

@lucacasonato lucacasonato added this pull request to the merge queue Apr 23, 2024
Merged via the queue into main with commit ddcb541 Apr 23, 2024
9 checks passed
@lucacasonato lucacasonato deleted the ban_import_assertiobs branch April 23, 2024 13:13
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.

Prevent publishing packages that use import assertions
2 participants