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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support webpack magic comments #197

Merged
merged 5 commits into from
Jun 2, 2021
Merged

feat: support webpack magic comments #197

merged 5 commits into from
Jun 2, 2021

Conversation

NozomuIkuta
Copy link
Contributor

@NozomuIkuta NozomuIkuta commented May 17, 2021

This PR is to support #71 in some way.

I just read through the code of this module once, so I may not be misunderstanding the implementation and making mistakes in approach. But, still, I will submit this PR with WIP in the title, so that everyone can point them out and/or have a discussion on my implementation.


According to Webpack's documentation, there are 6 kinds of magic comments supported by Webpack 4+, and they can be combined.

  • webpackInclude
  • webpackExclude
  • webpackChunkName
  • webpackMode
  • webpackPrefetch
  • webpackPreload

In the context of component importing, I think it would be enough to support only preferch and preload (at least to resolve #71).


The author of the issue wants to set magic comment per component in the place where it's used.

<LazyHeader /* webpackPreload: true */ />

However, it will be hassle to detect them in build pipeline.
In addition, I guess it is likely that lazy components are wanted to be prefetched or preloaded, regardless of the places they are used.
So, IMO, it will be reasonable to configure it before components are handled by the loader (or before they are imported).


My approach is to add 2 properties to objects of components array module option: prefetch and preload.
This would be simplest in terms of code and API change. In this way, all the components under specific directories are configured same.

2nd possible approach will be to provide a way to configure magic comments per component.
In this approach, API would get to be a little bit complicated.

3rd approach will be to statically add magic comments in import statement in the loader.
I know "no option" is best design sometimes, but I'm not sure it's now.

I do appreciate any feedback.


P.S.

This is my very first PR to Nuxt Component modules, after Nuxt Content. 馃懚
I hope that my PR will be consequently merged and used by Nuxt Core.


TODO

  • Verify approach
  • Take review and fix code
  • Add tests
  • Update documentation

@pi0
Copy link
Member

pi0 commented May 17, 2021

Seems a nice improvement 馃憤 /cc @clarkdo Confirming if nuxt supports webpack hints for server-renderer manifest.

@pi0 pi0 requested a review from clarkdo May 17, 2021 15:42
@clarkdo
Copy link
Member

clarkdo commented May 17, 2021

I鈥檒l do some testing and double check in nuxt for SSR

Copy link
Member

@clarkdo clarkdo left a comment

Choose a reason for hiding this comment

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

Just tested, it works ok.

image
image
image

return ` ${c.pascalName.replace(/^Lazy/, '')}: () => import('../${relativeToBuild(c.filePath)}' /* webpackChunkName: "${c.chunkName}" */).then(c => wrapFunctional(${exp}))`
const magicComments = [
`webpackChunkName: "${c.chunkName}"`,
c.prefetch ? `webpackPrefetch: ${c.prefetch ? 'true' : 'false'}` : false,
Copy link
Member

Choose a reason for hiding this comment

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

webpackPrefetch and webpackPreload expected true or a number, so I think we can remove false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for pointing out!
I made the parts more type-strict.

@NozomuIkuta
Copy link
Contributor Author

@pi0 @clarkdo

I modified the code according to @clarkdo 's comment, and updated README.

It seems that plugin changes on template files doesn't have unit tests.

I will remove the WIP prefix from the PR title, but is there anything that I should do?

@NozomuIkuta NozomuIkuta changed the title [WIP] feat: support webpack magic comments feat: support webpack magic comments May 18, 2021
README.md Outdated
- Type: `Boolean/Number`
- Default: `false`

These properties are used for Wepack's magic comments, learn more in [Wepack's official documentation](https://webpack.js.org/api/module-methods/#magic-comments).
Copy link
Member

@pi0 pi0 May 18, 2021

Choose a reason for hiding this comment

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

It would be nice to mention it is effective only when Lazy prefix is used. During development, we don't enable loader so it affects every component but in production, detected components are directly imported via installComponents instead of async imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I updated README by 6c33178.

Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

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

Overall looks good to me 馃憤 Doing for another local try before merge.

@NozomuIkuta NozomuIkuta requested a review from pi0 May 19, 2021 03:57
@NozomuIkuta
Copy link
Contributor Author

Sorry for newbie question, but what is difference between templates: index.js and plugin.js?
Should I modify index.js too?

As far as I see, the file isn't invoked from anywhere...?

@pi0
Copy link
Member

pi0 commented Jun 2, 2021

@NozomuIkuta index file is mainly for custom integrations like testing and not used directly. But would be nice if you can add it to that file too so we won't forget about if used in the future.

Mergin to avoid stalling. PR for improvement welcome and thanks <3

@pi0 pi0 merged commit 4e88e8f into nuxt:main Jun 2, 2021
pi0 pushed a commit that referenced this pull request Jun 3, 2021
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.

Webpack Preload comments for LazyLoad (And comments in general)
3 participants