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.

Sign up for GitHub

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

Jump to bottom

feat: support experimental inline match resource #2046

Merged
merged 3 commits into from Jun 1, 2023
Merged

feat: support experimental inline match resource #2046

merged 3 commits into from Jun 1, 2023

Conversation

h-a-n-a
Copy link
Contributor

@h-a-n-a h-a-n-a commented May 20, 2023 •
edited

This PR leverages inline match resource, a new technique provided by Webpack@5
to process modules, which allows loader developers to write more ergonomic code
and make the code easier to maintain. From now on, you can enable
options.experimentalInlineMatchResource of vue-loader to give it a try.

Other minor changes:

  1. Dispatch VueLoaderPlugin dynamically to decouple webpack and support webpack-like plugin systems.
  2. Webpack@5 experiments.css support

Note:
The ideal solution with inline match resource is to get rid of the VueLoaderPlugin completely,
I will try to optimize this in the future.

@h-a-n-a
Copy link
Contributor Author

h-a-n-a commented May 22, 2023

@sodatea Hi, I haven't finished the final touch yet. But I have some questions about how this new feature will be integrated into the codebase.
This feature, to me, is supposed to be enabled via some options like experimentalInlineMatchResource or something like this. However, this function affects both the implementation of plugin and loader, so I guess it will be better to configure in both plugin and loader(from the perspective of developer), or there might be some hacks. Which one do you prefer?
Another question is the option experiments.css provided by Webpack. Currently, I decide to detect the option automatically, is it necessary to provide another option for that?
Thank you for the help in advance!

@sodatea
Copy link
Member

sodatea commented May 23, 2023

Which one do you prefer?

I'm okay with configuring it in both the plugin and loader.
Most users won't need to configure the loader/plugin by themselves anyway.

Currently, I decide to detect the option automatically, is it necessary to provide another option for that?

I think automatic detection is better.

@h-a-n-a h-a-n-a marked this pull request as ready for review May 23, 2023 04:51
@h-a-n-a
Copy link
Contributor Author

h-a-n-a commented May 23, 2023

@sodatea
Firstly, thank you for the help! This PR is ready to review. I decide to leverage the old hack to make all possible configuration to vue-loader itself. So options.experimentalInlineMatchResource will only be exposed to the loader option (I think that will be enough as most of the user only want compilation-wise options).
Secondly, the old rule modification will work with either inline match resource enabled or not, but only the new rule modification will apply with inline match resource enabled.
One more thing, webpack's experiments.css has some limitations with the current implementation, as inline and module will not work (requires javascript), however scoped is working as intended. Hope I can add some tests in the future PR.

@h-a-n-a h-a-n-a changed the title feat: inline match resource feat: support experimental inline match resource May 23, 2023
hardfist
hardfist reviewed May 23, 2023
View reviewed changes
src/index.ts Outdated
} = loaderContext

const webpackVersion = _compiler?.webpack?.version
const isWebpack5 = webpackVersion && webpackVersion[0] > '4'
Copy link

Choose a reason for hiding this comment

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

webpackVersion[0] > '4' is dangerous when webpackVersion > 10.x.y, I thinks direct equal test should be better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is copied from the original implementation. I changed it to a relatively better one.

@h-a-n-a h-a-n-a force-pushed the feat/inline-match-resource-new branch 2 times, most recently from 8eb7ae2 to 42e4747 Compare May 29, 2023 06:41
@h-a-n-a
feat: init
fc9b73d 
feat: finish inline match resource

chore: cleanup

chore: cleanup

feat: add experimental css support

feat: dispatch plugin dynammically

feat: support option `experimentalInlineMatchResource`

chore: enable test on CI

chore: disable sourceMap generation

]

docs: add option doc

docs: correct version

fix: fix lang matching

feat: use a relatively better version test

fix: fix incorrect match with `experiments.css` enabled

fix: more accurate when matching styles

feat: optimize for `Rule.loader`

fix: import
@h-a-n-a h-a-n-a force-pushed the feat/inline-match-resource-new branch from 42e4747 to fc9b73d Compare May 29, 2023 06:42
@h-a-n-a
Copy link
Contributor Author

h-a-n-a commented May 29, 2023

@sodatea Hi, CI workflow has been fixed. Would you please take a look again? :)

sodatea
sodatea reviewed Jun 1, 2023
View reviewed changes
src/util.ts Outdated
resourcePath: string,
resourceQuery?: string,
lang?: string,
additionalLoaders?: string[]
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any usage of this parameter. What's it reserved for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not related. I will remove this.

sodatea
sodatea reviewed Jun 1, 2023
View reviewed changes
src/pluginWebpack5.ts
pitcher,
...rules.filter((rule) => !vueLoaderRules.includes(rule)),
templateCompilerRule,
...clonedRules,
Copy link
Member

Choose a reason for hiding this comment

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

I thought the intention of using inline match resource was to get rid of cloning rules. Can we remove it here?

Copy link
Contributor Author

@h-a-n-a h-a-n-a Jun 1, 2023 •
edited

Choose a reason for hiding this comment

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

I think what blocks us from removing this currently is the lang query for us. templateCompilerRule seems depending on the result from the remaining rules. But I will try this in the future PR. Ideally, we could get rid of the VueLoaderPlugin in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. Supporting template pre-processors is a bit challenging. We can implement that later. Thanks!

@sodatea sodatea merged commit 3149f6d into vuejs:main Jun 1, 2023
3 checks passed
@h-a-n-a h-a-n-a deleted the feat/inline-match-resource-new branch June 1, 2023 09:07
@h-a-n-a h-a-n-a mentioned this pull request Jun 6, 2023
Closed
freddy38510 pushed a commit to freddy38510/vue-loader that referenced this pull request Sep 6, 2023
@h-a-n-a @freddy38510
feat: support experimental inline match resource (vuejs#2046)
cd07b1c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hardfist hardfist hardfist left review comments

@sodatea sodatea sodatea approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@h-a-n-a @sodatea @hardfist