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(vue-app): support configurable features #6287

Merged
merged 20 commits into from
Sep 5, 2019
Merged

Conversation

pimlie
Copy link

@pimlie pimlie commented Aug 23, 2019

This PR is only a draft because we didn't discuss this yet before. Feature-wise I think its complete, maybe just need to add some tests.

Types of changes

  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

The Nuxt Vue-app has quite a bit of features which until this PR were always included, whether you used them or not. This PR defines a list of features which are used to either include or omit the corresponding code in the Vue-app templates. This makes it possible for a user to further optimize their bundle sizes.

Here is the list of features which are toggleable:

  features: {
    router: true, // not implemented
    store: true,
    layouts: true,
    meta: true,
    middleware: true,
    transitions: true,
    deprecations: true,
    validate: true,
    asyncData: true,
    fetch: true,
    clientOnline: true,
    clientPrefetch: true,
    clientUseUrl: false, // this is a bit of an odd one, but using URL should eg be ok for modern mode already
    componentAliases: true,
    componentClientOnly: true
  }

I've included a copy of the hello world example app with all (supported) features disabled.

App bundle size of Hello world app
image

App bundle size of Minimal-features Hello world app:
image
image

This PR was also partially given in by the benchmark discussions on discord. You could argue that disabling all these settings is not very usefull and I will probably agree with that. But giving users the possiblity to further optimize their bundle sizes is a nice feature I think and worth the extra work with regards to maintaining the vue-app templates.

Disabling features is considered highly experimental and for advanced users only.

Note: The above bundle size of the Hello world app has already improved slightly compared to the current dev branch. This is mainly due to some optimisations in ./utils.js. Eg urlJoin is/was always included in the app bundle but thats only used in server.js

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. (PR: #)
  • I have added tests to cover my changes (if not applicable, please state why)
  • All new and existing tests are passing.

@pimlie pimlie requested a review from a team August 23, 2019 13:47
packages/builder/src/builder.js Outdated Show resolved Hide resolved
packages/vue-app/src/index.js Outdated Show resolved Hide resolved
packages/vue-app/template/App.js Outdated Show resolved Hide resolved
packages/vue-renderer/src/renderers/ssr.js Outdated Show resolved Hide resolved
@manniL
Copy link
Member

manniL commented Aug 23, 2019

Amazing idea 👍

fix builder.generate test

implement review suggestions

support features.layouts

support features.store (so store can exists but still be disabled)
@pimlie
Copy link
Author

pimlie commented Aug 24, 2019

Added the suggestions and implemented store/layout features. Also had some wrong/old features in the nuxt.config of the minimal example, so size is even smaller now (image of app bundle size is updated).

@pi0
Copy link
Member

pi0 commented Aug 24, 2019

Really nice enhancement. I wish we could also support disabling router. But seems not an easy task for initial PR.

@codecov-io
Copy link

codecov-io commented Aug 24, 2019

Codecov Report

Merging #6287 into dev will increase coverage by 0.02%.
The diff coverage is 97.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #6287      +/-   ##
==========================================
+ Coverage   95.71%   95.74%   +0.02%     
==========================================
  Files          79       79              
  Lines        2663     2678      +15     
  Branches      682      691       +9     
==========================================
+ Hits         2549     2564      +15     
  Misses         98       98              
  Partials       16       16
Flag Coverage Δ
#e2e 100% <ø> (ø) ⬆️
#fixtures 50.82% <32.43%> (+0.12%) ⬆️
#unit 92.38% <94.59%> (ø) ⬆️
Impacted Files Coverage Δ
packages/builder/src/context/template.js 100% <ø> (ø) ⬆️
packages/vue-app/src/index.js 0% <ø> (ø) ⬆️
packages/config/src/config/_app.js 100% <ø> (ø) ⬆️
packages/builder/src/builder.js 99.64% <100%> (+0.01%) ⬆️
packages/vue-renderer/src/renderers/spa.js 87.87% <100%> (+0.18%) ⬆️
packages/vue-renderer/src/renderers/ssr.js 94.02% <92.85%> (+0.37%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c010add...f1bfdf9. Read the comment docs.

@pimlie pimlie marked this pull request as ready for review August 24, 2019 10:10
@pimlie
Copy link
Author

pimlie commented Aug 24, 2019

I looked into making the router configurable as well but almost every function in client.js makes use of from / to params and it seems just to much integrated into all the logic. Dont think it will be an easy task to support disabling the router in the current code. A complete rewrite of client.js with optional router disabling in mind will probably be faster.

manniL
manniL previously approved these changes Aug 25, 2019
@atinux
Copy link
Member

atinux commented Aug 27, 2019

Great work @pimlie, I had this in mind for Nuxt 3.

Ideally, the features enable themselves when used (when detecting the directory associated or defined in the config). Also, I am afraid that some module may not work if the user disable some features 🤔

@pimlie
Copy link
Author

pimlie commented Aug 27, 2019

Ideally, the features enable themselves when used (when detecting the directory associated or defined in the config).

Well, yes/no. I can imagine there are also times you just want to disable a feature regardless whether you have certain files in your project. Eg with the current store implementation I did you can have a store module in your project, but if you toggle the feature to false it will still be disabled.

Also, I am afraid that some module may not work if the user disable some features 🤔

True, for the moment thats really up to the user to think about. Thats why I mentioned experimental and for advanced users only.

I dont think we should launch this pr as a great new feature anyway, not as long as we cant provide a router feature. So I would propose to do a silent launch, learn from it and then improve by maybe taking the approach you had in mind for v3

@manniL
Copy link
Member

manniL commented Aug 27, 2019

Well, yes/no. I can imagine there are also times you just want to disable a feature regardless whether you have certain files in your project. Eg with the current store implementation I did you can have a store module in your project, but if you toggle the feature to false it will still be disabled.

Definitely, the user prefence should take precedence! But disabling/enabling features if certain conditions are present and the user didn't set the config otherwise would be nice.

useUrl: true
},
components: {
aliases: false,
Copy link
Member

Choose a reason for hiding this comment

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

what do you think about flattened options like compoentAliases? This way we don't need to always worry about falsy value in the path (Like components: false)

pi0
pi0 previously approved these changes Sep 1, 2019
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.

A REALLY REALLY nice improvement to support minimal builds for nuxt. Can't wait to also have router disabling support. With nuxt 3 we can have nuxt to be used like:

  • Create app.vue
  • npx nuxt dev
  • TADA! A fully minimal/universal/efficient vue app is ready!

I think we should be better to merge this ASAP to avoid merge conflicts.

@pimlie pimlie dismissed stale reviews from pi0 and manniL via 6f1b26b September 2, 2019 07:42
@pi0 pi0 requested review from clarkdo, atinux and a team September 2, 2019 10:09
@pimlie
Copy link
Author

pimlie commented Sep 2, 2019

@clarkdo Thanks, could you please verify if I implemented it correctly?

And I would appreciate your/anyone's help with resolving the error with unicode-base, dont really understand why that now gives an error? The file exists on the fs and is listed in the manifest, does one of you maybe have any idea why jsdom cant find it? Only change for that test is that I added modern: 'server' in nuxt.config.

--edit--
It seems that in none of the modern test renderAndGetWindow is used, guess thats on purpose? I could use rp as well but that doesnt encode the unicode char automatically

@clarkdo
Copy link
Member

clarkdo commented Sep 2, 2019

@pimlie Looks perfect, you can double verify it by having a look at analyze report.

@pi0 pi0 self-requested a review September 3, 2019 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants