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

feat: support experimental inline match resource #2046

Merged
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

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!

@haoqunjiang
Copy link
Member

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
src/index.ts Outdated
} = loaderContext

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

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
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? :)

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.

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

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!

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.

3 participants