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

fix: improve watching experience for generated files #6257

Merged
merged 7 commits into from
Aug 21, 2019
Merged

fix: improve watching experience for generated files #6257

merged 7 commits into from
Aug 21, 2019

Conversation

pi0
Copy link
Member

@pi0 pi0 commented Aug 20, 2019

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

Nuxt has a frustrating DX issue when a page file is deleted or renamed. Webpack starts to rebuild as soon as the file is changed while still generated files (like .nuxt/router.js) are referring to the old file. This is happening because we debounce generateRoutesAndFiles function (also there is a little generation/fs process time) but webpack rebuilds as soon as possible. Causing a failing intermediate build like this:

image

This PR set a longer default for aggregateTimeout (200ms + ~800ms of generating process). That fixes the problem in normal cases when generation takes less than 800ms: (previously deadline was ~100ms)

image

Webpack watch will be also suspended during generate files and resumed/invalidated thanks to pausing/resume introduced webpack/webpack#9214 (@jkzing ❤️)

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.

@codecov-io
Copy link

codecov-io commented Aug 20, 2019

Codecov Report

Merging #6257 into dev will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #6257      +/-   ##
==========================================
- Coverage   95.76%   95.73%   -0.03%     
==========================================
  Files          80       80              
  Lines        2666     2673       +7     
  Branches      687      689       +2     
==========================================
+ Hits         2553     2559       +6     
- Misses         97       98       +1     
  Partials       16       16
Flag Coverage Δ
#e2e 100% <ø> (ø) ⬆️
#fixtures 50.72% <85.71%> (+0.09%) ⬆️
#unit 92.44% <100%> (-0.02%) ⬇️
Impacted Files Coverage Δ
packages/config/src/config/_common.js 100% <ø> (ø) ⬆️
packages/webpack/src/builder.js 92.78% <100%> (+0.23%) ⬆️
packages/builder/src/builder.js 99.61% <100%> (ø) ⬆️
packages/vue-renderer/src/renderer.js 93.49% <0%> (-0.82%) ⬇️

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 6ac5544...6dc98f6. Read the comment docs.

@pi0 pi0 requested review from clarkdo, Atinux, kevinmarrec and a team August 20, 2019 17:30
clarkdo
clarkdo previously approved these changes Aug 20, 2019
@Atinux
Copy link
Member

Atinux commented Aug 21, 2019

Expecting the strange failing test 🤔
Awesome PR :)

@pi0 pi0 added this to To do in Nuxt v2.10 Aug 21, 2019
@pi0 pi0 moved this from To do to In progress in Nuxt v2.10 Aug 21, 2019
@pi0 pi0 self-assigned this Aug 21, 2019
@pi0 pi0 requested review from clarkdo, pimlie and a team August 21, 2019 17:33
@pi0 pi0 moved this from In progress to Ready to Review in Nuxt v2.10 Aug 21, 2019
Copy link

@pimlie pimlie left a comment

Choose a reason for hiding this comment

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

Looks good, hope this improves DX indeed!

@pi0 pi0 merged commit 81b92b6 into dev Aug 21, 2019
@pi0 pi0 deleted the fix/watch branch August 21, 2019 19:04
@pi0 pi0 moved this from Ready to Review to Done in Nuxt v2.10 Aug 21, 2019
@pi0 pi0 mentioned this pull request Aug 22, 2019
Atinux pushed a commit that referenced this pull request Aug 28, 2019
* [release]

* fix(vue-app): enforce default css when used with frameworks (#6255)

* chore(deps): update dependency serialize-javascript to ^1.8.0 (#6254)

* fix(config): provide typescript backward compatibility (#6258)

* fix(vue-app): only overwrite leave listener when none provided or without done arg (#6264)

* fix(nuxt-start): add missing `vue-client-only` dependency (#6267)

* fix(server): treat `https: null` as `https: undefined` (#6265)

* chore(deps): update devdependency rollup to ^1.20.0 (#6268)

* fix: improve watching experience for generated files (#6257)

* refactor(core): use hable (#6271)

* test: disable terser/minify by default (#6290)

* chore(deps): update devdependency rollup to ^1.20.1 (#6282)

* chore(cli): accept hooks (#6274)

* chore(deps): update devdependency rollup-plugin-alias to v2 (#6281)

* fix(builder): apply overrides from app dir only (#6283)

[release]

* feat: function watchQuery (#6245)

* Revert "feat: function watchQuery (#6245)" (#6296)

This reverts commit 3c61830.

* chore(deps): update devdependency rollup to ^1.20.2 (#6295)



Co-authored-by: Renovate Bot <bot@renovateapp.com>

* test(vue-app): add template compiler helper (#6299)

* test: add jest roots for less greedy test search (#6300)

* test: remove unnecessary generate (#6301)

* fix: update the minimal require node version in distributions (#6310)

* chore(deps): update devdependency babel-eslint to ^10.0.3 (#6304)

* chore(deps): lock file maintenance (#6305)

* chore: upgrade circleci config to v2.1 (#6312)

* test: add test for modern bundle size (#6302)

* test: add client-only test to basic fixture (#6315)

test: check for no-ssr deprecation warning
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Nuxt v2.10
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants