Ensure npm install runs once at a time #9
Conversation
Current coverage is 97.82% (diff: 100%)@@ master #9 diff @@
==========================================
Files 1 1
Lines 41 46 +5
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 40 45 +5
Misses 1 1
Partials 0 0
|
zakj
force-pushed
the
single-npm-process
branch
2 times, most recently
from
74d5d2c
to
8a84200
Compare
|
Ha, you caught me trying to avoid threaded tests. ;) I'm glad you did, though—I think I've arrived at a slightly better implementation while making it testable. Let me know if there's anything else I can fix up, and thanks so much for this plugin (and your work on Lektor itself)! |
| reporter.report_generic('Running npm install') | ||
| webpack_root = os.path.join(self.env.root_path, 'webpack') | ||
| portable_popen(['npm', 'install'], cwd=webpack_root).wait() | ||
| if self.npm_lock.acquire(False): |
There was a problem hiding this comment.
Can you specify blocking=False here? I had to look up the documentation for threading.Lock to understand what this False was for.
There was a problem hiding this comment.
Sadly, no:
>>> import threading
>>> threading.Lock().acquire(blocking=False)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: acquire() takes no keyword arguments
Would an inline comment be useful?
if self.npm_lock.acquire(False): # non-blocking| webpack_root = os.path.join(self.env.root_path, 'webpack') | ||
| portable_popen(['npm', 'install'], cwd=webpack_root).wait() | ||
| else: | ||
| self.npm_lock.acquire() |
There was a problem hiding this comment.
Why are you acquiring the lock here, if you're not doing anything with it?
There was a problem hiding this comment.
To maintain the behavior that this method does not exit until the npm install process is complete, since callers expect to have a complete set of installed dependencies after calling it. (I.e., if you can acquire the lock, run npm and then release it. If you can't acquire the lock, wait for it to be released before exiting.) Otherwise you might end up attempting to run webpack before it's installed.
zakj
force-pushed
the
single-npm-process
branch
from
8a84200
to
f33edf4
Compare
On starting up the Lektor server, the server_spawn and before_build_all events both fire, each one spawning `npm install`. This ensures that only one runs at a time.
On starting up the Lektor server, the server_spawn and before_build_all events both fire, each one spawning
npm install. This ensures that only one runs at a time.