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

parse import assertions #12278

Merged
merged 8 commits into from Jul 16, 2021
Merged

parse import assertions #12278

merged 8 commits into from Jul 16, 2021

Conversation

xtuc
Copy link
Member

@xtuc xtuc commented Dec 27, 2020

Refs #11917

What kind of change does this PR introduce?

Parsing import assertions syntax

Did you add tests for your changes?
Yes, one. Most of the tests are in https://github.com/xtuc/acorn-import-assertions/tree/main/test/fixtures

Does this PR introduce a breaking change?
no

What needs to be documented once your changes are merged?
probably not now, import assertions doesn't emit runtime code yet

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

@alexander-akait
Copy link
Member

Tests are broken 😞

@xtuc
Copy link
Member Author

xtuc commented Dec 28, 2020

@alexander-akait yes noticed that too. I was wondering if those failures are related to my change? I can't see the detail of the failures but they are mismatching stats? I'll try to run them locally again

@xtuc xtuc force-pushed the sven/import-assertions branch 3 times, most recently from 1eaa9a1 to 7dab9d7 Compare December 28, 2020 23:17
@xtuc
Copy link
Member Author

xtuc commented Dec 29, 2020

@vankop vankop closed this Dec 29, 2020
@vankop vankop reopened this Dec 29, 2020
@alexander-akait
Copy link
Member

@vankop thanks)

@xtuc
Copy link
Member Author

xtuc commented Dec 29, 2020

right, sorry I forgot about that trick! thanks

@xtuc
Copy link
Member Author

xtuc commented Jan 5, 2021

@sokra could you please take a look at the PR? the idea is that we allow parsing and stripping out import assertions for now.
Next steps would be to emit runtime code to check the type of dynamically loaded resources.

@sokra
Copy link
Member

sokra commented Jan 5, 2021

Next steps would be to emit runtime code to check the type of dynamically loaded resources.

JSON is not loaded dynamically but embedded into the JS file. As it's parsed during build it's already verified that it's really JSON and not JS or something else.

I think what we can do it to set the module type for imports with assert { type: "json" } to JSON so they go through the JSON parser. This could be done by allowing an assert condition in module.rules and adding a default rule to set type: "json" for assert: { type: "json" } (module.defaultRules: [ ..., { assert: { type: "json" }, type: "json" }] in lib/config/defaults.js).

For a assert condition we need to pass the assert value from HarmonyImportDependencyParserPlugin -> ModuleDependency.assert (getResourceIdentifier need to have assert too) -> NormalModuleFactory (add assert to ruleSetCompiler, and pass the value when executing it, get it from dependencies[0].assert).

@xtuc
Copy link
Member Author

xtuc commented Jan 19, 2021

JSON is not loaded dynamically but embedded into the JS file. As it's parsed during build it's already verified that it's really JSON and not JS or something else.

I was thinking of loading dynamically remote resources. Like: import("https://a.com/great-json.php"), the runtime should ensure that the response is indeed JSON. But we can do that later.

Thanks for your pointers, i'll do that change.

@xtuc xtuc changed the title parse import assertions WIP parse import assertions Jan 24, 2021
@xtuc
Copy link
Member Author

xtuc commented Jan 24, 2021

@sokra I believe I have implemented what you suggested. One test doesn't pass yet and we probably want to show a code frame when the assertion aren't met, that's still WIP. Could you please do an early review?

package.json Outdated Show resolved Hide resolved
@xtuc
Copy link
Member Author

xtuc commented Feb 21, 2021

@sokra could you please review the change so far? I believe it matches what you suggested.

@sokra sokra merged commit df02bc6 into webpack:main Jul 16, 2021
@sokra
Copy link
Member

sokra commented Jul 16, 2021

Thanks

@xtuc xtuc deleted the sven/import-assertions branch July 16, 2021 15:20
@xtuc
Copy link
Member Author

xtuc commented Jul 16, 2021

Great, thanks for your help!

@redonkulus
Copy link

@sokra is #13814 related to this merge?

@alexander-akait
Copy link
Member

@redonkulus yep

@sodatea
Copy link
Contributor

sodatea commented Dec 30, 2021

Should assert be mentioned in https://webpack.js.org/configuration/module/#rule ?
I've seen it used in css-loader examples but can't find it in the webpack documentation.

@alexander-akait
Copy link
Member

@sodatea Yes, feel free to open an issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants