feat: lock project during build or generate #4985

pimlie · 2019-02-08T11:23:35Z

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

This PR adds support for locking as proposed in nuxt/rfcs#23

I was not sure about the usefulness of locking when running nuxt dev, but will add it if you think it should also support it.

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.

packages/cli/src/utils/index.js

codecov-io · 2019-02-08T11:30:30Z

Codecov Report

Merging #4985 into dev will increase coverage by 1.01%.
The diff coverage is 97.77%.

@@            Coverage Diff             @@
##              dev    #4985      +/-   ##
==========================================
+ Coverage   95.05%   96.07%   +1.01%     
==========================================
  Files          72       73       +1     
  Lines        2425     2469      +44     
  Branches      616      624       +8     
==========================================
+ Hits         2305     2372      +67     
+ Misses        100       82      -18     
+ Partials       20       15       -5

Impacted Files	Coverage Δ
packages/cli/src/utils/index.js	`96.72% <0%> (+35.05%)`	⬆️
packages/utils/src/locking.js	`100% <100%> (ø)`
packages/cli/src/command.js	`98.87% <100%> (+0.15%)`	⬆️
packages/cli/src/commands/generate.js	`100% <100%> (ø)`	⬆️
packages/cli/src/commands/build.js	`86.36% <100%> (+1.36%)`	⬆️
packages/cli/src/utils/formatting.js	`80.95% <0%> (+9.52%)`	⬆️

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 04cdd60...6036de5. Read the comment docs.

pimlie · 2019-02-08T11:31:21Z

packages/cli/src/commands/generate.js

@@ -44,6 +45,25 @@ export default {
    config.build.analyze = false

    const nuxt = await cmd.getNuxt(config)
+
+    if (cmd.argv.lock) {
+      cmd.setLock(await createLock({


Is it useful at all to create separate locks for the build and generate phases?

@pimlie As "build" is also included in the generate step, we could remove the lock after build for nuxt build and after generate for nuxt generate (if we can distinguish)

We could distinguish by either using different id's or using different lock dir's. Besides as a proof-of-concept I thought having separate locks might be useful in an edge-case where the generate phase takes quiet a while to run but in the mean time you want to already rebuild the project because you also want to run the nuxt server but with a slightly different configuration.

If you want to distinguish between (similar) locks started by separate commands we could remove the lock id from the actual lockPath but then we should add a mapping between lockPaths and id's.

pimlie · 2019-02-08T11:35:45Z

packages/utils/src/locking.js

+  return Object.assign({}, defaultLockOptions, options)
+}
+
+export function createLockPath({ id = 'nuxt', dir, root }) {


I added the id due to a remark of pi0 in the rfc about having separate locks for separate commands. I agree that that wouldnt be very useful, but having the possibility to have different locks on the same dir could be useful. Also the id is printed in the error when a lock already exists and could be useful to have a clearer understanding of why the lock exists in the first place

pimlie · 2019-02-08T11:42:50Z

packages/utils/src/locking.js

+export const lockPaths = new Set()
+
+export const defaultLockOptions = {
+  stale: 30000,


There is no direct need for checking the validity of a lockfile when a lock already exists, proper-lockfile sets a timer which updates the lockfile regularly and if a lockfile exists it already checks for staleness. See here for more info: https://github.com/moxystudio/node-proper-lockfile#comparison

I have set a default value for staleness of 30s, but maybe thats a bit much and we can just use the default value.

pi0

Overall LGTM

packages/cli/src/command.js

packages/cli/src/utils/index.js

fix: getGenerator mocking chore: add test for checking double locking by generate

pimlie · 2019-02-09T15:04:25Z

packages/cli/src/command.js

@@ -1,4 +1,4 @@
-


Maybe we could add an eslint rule for this?

husayt · 2019-03-04T13:04:13Z

This is causing an error when building on netlify

12:26:56 PM: webpackbar 12:26:56 ℹ Compiling Client
12:27:18 PM: /opt/build/repo/node_modules/proper-lockfile/lib/lockfile.js:181
12:27:18 PM:         onCompromised: (err) => { throw err; },
12:27:18 PM:                                   ^
12:27:18 PM: Error: Unable to update lock within the stale threshold
12:27:18 PM:     at options.fs.utimes (/opt/build/repo/node_modules/proper-lockfile/lib/lockfile.js:109:21)
12:27:18 PM:     at FSReqWrap.oncomplete (fs.js:135:15)

pimlie · 2019-03-04T13:09:41Z

You can run nuxt with --no-lock to disable locking.

Any idea if this is netlify specific or ci generic? Maybe we should turn locking of by default then when running on ci?

pimlie · 2019-03-05T09:59:33Z

@husayt Could you maybe create a small reproduction that triggers the error? I tried adding timers in a build:before hook and asyncData (see here: https://github.com/pimlie/nuxt-issues/) but netlify still builds my site

husayt · 2019-03-06T15:09:08Z

@pimlie seems like it is working now, but not sure if it is latest version or some of my workarounds. I will keep my eye on it and will let you know, if anything.

Thank you

pimlie · 2019-03-06T18:05:35Z

@husayt Thanks for the feedback. It seems it was not just you, @aldarund also had issues with it on wercker. There appears to be an issue with proper-lockfile due to the staleness timer not updating the lock under heavy cpu load, but there might be another issue as well. Still investigating, so please let me know here or on discord if it happens again (and if you can help with a small reproduction that always triggers on netlify that would be very helpful)

husayt · 2019-03-13T17:26:51Z

I have it again, this time with nuxt-ts ~~and --no-lock doesn't help either~~

To get no lock working I had to use -- --no-lock

pimlie added 5 commits February 8, 2019 10:21

feat: lock project during build/generate

80a0e05

Merge branch 'dev' into feat-lock-file

d0fc554

chore: fix tests

9386469

fix: also release lockPath from lockPaths set

7a19fed

fix: remove merge issue

45217d1

pimlie commented Feb 8, 2019

View reviewed changes

packages/cli/src/utils/index.js Show resolved Hide resolved

pimlie commented Feb 8, 2019

View reviewed changes

fix: improve coverage

932fc69

pi0 reviewed Feb 8, 2019

View reviewed changes

packages/cli/src/command.js Show resolved Hide resolved

packages/cli/src/utils/index.js Show resolved Hide resolved

galvez assigned manniL and pimlie and unassigned manniL Feb 8, 2019

pimlie added 3 commits February 9, 2019 15:23

chore: add warning about jest.mock error

96e938b

chore: lower state timeout

eada36e

fix: remove old lock when setting new lock and print warning

90595ba

fix: getGenerator mocking chore: add test for checking double locking by generate

pimlie commented Feb 9, 2019

View reviewed changes

packages/cli/src/command.js

@@ -1,4 +1,4 @@

Copy link

Author

pimlie Feb 9, 2019

Choose a reason for hiding this comment

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

Maybe we could add an eslint rule for this?

manniL reacted with thumbs up emoji

pi0 previously approved these changes Feb 11, 2019

View reviewed changes

Merge branch 'dev' into feat-lock-file

83422b4

Atinux dismissed pi0’s stale review via 83422b4 February 26, 2019 13:34

Atinux requested review from pi0, manniL and Atinux February 26, 2019 13:34

Atinux approved these changes Feb 26, 2019

View reviewed changes

Merge branch 'dev' into feat-lock-file

6036de5

pi0 approved these changes Mar 3, 2019

View reviewed changes

pi0 merged commit 4e51723 into nuxt:dev Mar 3, 2019

pimlie deleted the feat-lock-file branch March 4, 2019 13:22

pi0 mentioned this pull request Mar 14, 2019

v2.5.0 #5237

Merged

danielroe mentioned this pull request Mar 23, 2019

Unable to update lock within the stale threshold nuxt/vercel-builder#19

Closed

danielroe added the 2.x label Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: lock project during build or generate #4985

feat: lock project during build or generate #4985

pimlie commented Feb 8, 2019

codecov-io commented Feb 8, 2019 •

edited

pimlie Feb 8, 2019

manniL Feb 8, 2019

pimlie Feb 8, 2019

pimlie Feb 8, 2019

pimlie Feb 8, 2019

pimlie Feb 8, 2019

pi0 left a comment

pimlie Feb 9, 2019

husayt commented Mar 4, 2019

pimlie commented Mar 4, 2019

pimlie commented Mar 5, 2019

husayt commented Mar 6, 2019 •

edited

pimlie commented Mar 6, 2019

husayt commented Mar 13, 2019 •

edited

feat: lock project during build or generate #4985

feat: lock project during build or generate #4985

Conversation

pimlie commented Feb 8, 2019

Types of changes

Description

Checklist:

codecov-io commented Feb 8, 2019 • edited

Codecov Report

pimlie Feb 8, 2019

Choose a reason for hiding this comment

manniL Feb 8, 2019

Choose a reason for hiding this comment

pimlie Feb 8, 2019

Choose a reason for hiding this comment

pimlie Feb 8, 2019

Choose a reason for hiding this comment

pimlie Feb 8, 2019

Choose a reason for hiding this comment

pimlie Feb 8, 2019

Choose a reason for hiding this comment

pi0 left a comment

Choose a reason for hiding this comment

pimlie Feb 9, 2019

Choose a reason for hiding this comment

husayt commented Mar 4, 2019

pimlie commented Mar 4, 2019

pimlie commented Mar 5, 2019

husayt commented Mar 6, 2019 • edited

pimlie commented Mar 6, 2019

husayt commented Mar 13, 2019 • edited

codecov-io commented Feb 8, 2019 •

edited

husayt commented Mar 6, 2019 •

edited

husayt commented Mar 13, 2019 •

edited