-
Notifications
You must be signed in to change notification settings - Fork 222
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
build: Check to see if npm dependencies are already up to date; #2801
Conversation
only issue npm install --install-links=false if package.json has been modified since last time.
for more information, see https://pre-commit.ci
Taskfile.yml
Outdated
# symlink. Refer to https://github.com/PRQL/prql/pull/1296. | ||
desc: Check to see if package.json has changed since we ran npm install | ||
dir: web/playground | ||
status: # if both are true, don't rebuild |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we replace this all with sources:
? https://taskfile.dev/usage/#by-fingerprinting-locally-generated-files-and-their-sources
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably. My concern is that the node_modules
folder has ~50,000 files. Computing a checksum over all those files might take nearly as much time as the npm install
I'll check thetimestamp
method to see how it performs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use it on package.json
though? Like we're doing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remarkably, the checksum option is fast - less than a second. It's also indistinguishable from the timestamp
method. So this commit switches to using checksums.
My test process was to modify package.json
(add a blank line) and run task run-playground
which calls npm install ...
I then ^C, and re-ran task run-playground
and the Task reports, task: Task "check-npm-cache" is up to date
Can you suggest any other tests? Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure if it's using the files under generates
; I couldn't completely decipher the docs:
For the checksum (default) or timestamp method to work, it is only necessary to inform the source files. When the timestamp method is used, the last time of the running the task is considered as a generate.
Taskfile.yml
Outdated
- npm start | ||
# | ||
|
||
check-npm-cache: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is great, very nice addition.
Can we rename this to something like install-playground
? It's doing more than just checking the cache.
Then let's merge! (edit: one note added below)
Actually one question — do we need to reinstall when rust files in Even if that is the case, we can still have a task like |
re: renaming - a good idea re: I'll keep bulling ahead... |
No prob — why don't we merge this with something like And then we can refactor this all when we upgrade rust and can do this as part of the standard build, which is cached itself. |
because that's what it does
I went with a task name of "install-playground-npm-dependencies". Then we need to figure out how to trigger the |
# install prql-js as the regular dependency, instead of creating a | ||
# symlink. Refer to https://github.com/PRQL/prql/pull/1296. | ||
- npm install --install-links=false | ||
- task: install-playground-npm-dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking this wouldn't be the default under run-playground
— there would be a separate task run-playground-npm-dependencies
The default needs to pick up the full path, otherwise it would be v confusing that it's using old rust code.
I'm not sure it worth investing much in us detecting the rust changes, since it might be difficult and it will be replaced by the proper solution when we can do #1836
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... This PR is meant as a convenience for people doing documentation. The current main
branch always runs the full npm install (taking ~1 minute), for both task run-web
and task run-playground
, even if I ^C the one, start the other, with no changes to any of the source files.
On further inspection, I see there's a task run-website
that does what I want. It simply serves out the (previously-built) static site. I have created a similar task run-pg
that people can use to avoid the npm step completely.
I believe it's useful to keep the separate install-playground-npm-dependencies
task because it factors out the (unexpected) --install-links
option into a single function.
Potential To-do's:
- Detect changes to
prql-compiler
source file ininstall-playground-npm-dependencies
- Regularize the task names
build-web
- builds the website (the currentbuild-web
)build-run-web
- builds and runs the website (the currentrun-web
)run-web
- run previously-built static site (the currentrun-website
)- similar names for build-playground, build-run-playground, run-playground
- similar names for run-book, just for completeness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(responding quickly, forgive me for brevity!)
build-web
- builds the website (the currentbuild-web
)
build-run-web
- builds and runs the website (the currentrun-web
)
run-web
- run previously-built static site (the currentrun-website
)
I've been referring to web
as all three — book / playground / website — and website
as just the static site (at prql-lang.org). Does that change your view at all?
Detect changes to
prql-compiler
source file ininstall-playground-npm-dependencies
As I say above — if you want to do this then you're welcome to, but it'll go away shortly when #1836 is unblocked (by Debian rust version...)
On further inspection, I see there's a
task run-website
that does what I want. It simply serves out the (previously-built) static site.
The run-
steps also build things IIUC. But yes OK; we could have run-
skip the building and have build-run
do both? (Though, again, soon building will be very cheap, so it might be a smaller change to just add your new task as a suffix of -cached
and then we refactor it out when we do #1836?)
@@ -300,6 +300,12 @@ tasks: | |||
- task: install-playground-npm-dependencies | |||
- npm start | |||
|
|||
run-pg: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is OK! I would have voted for something clearer — not obvious how pg
is different from playground
(though it is elegant in a way! Smaller and less descriptive, just like running vs building!!)
If it's OK, I'm going to merge, and then we do any follow-ups in a separate PR? |
@max-sixty This is great. I'll be delighted when building is cheap. In the mean time, I don't have to be grumpy waiting for |
I hadn't realized that PRQL#2801 stopped building the rust code by default. This changes it slightly so running `run-playground-cached` avoids building the rust code. It removes `run-pg`, since the JS build without the rust build takes just a few seconds
I hadn't realized that PRQL#2801 stopped building the rust code by default. This changes it slightly so running `run-playground-cached` avoids building the rust code. It removes `run-pg`, since the JS build without the rust build takes just a few seconds
New task "check-npm-cache" that avoids unnecessary long startup times.
Save a .npmtimestamp file in the node_modules directory after
npm install
. If a subsequent call to check-npm-cache shows the package.json is older, thenthere's no need to re-run
npm install
...