build: Refactor Taskfile.yml

[richb-hanover]

Refactor Taskfile.yml to provide these commands that build and serve their product:

• run website
• run playground
• run book

The new install-playground-dependencies-all task forces the full "npm install..." with all its options All the other tasks rely on install-playground-dependencies that checks if packages.json has been modified and re-runs the full "npm install..." as needed

Breaking change: tasks run-web and run-playground-cached are no longer needed and commented out

[max-sixty]

I think this is overall very good, but I think it has one distinct large disadvantage: that changing the rust code and running task run-playground will not cause a rebuild. Is that accurate? That seems quite important.

[max-sixty]

The comment & footnote in #2907 (comment) references an approach using webpack directly, which I think would provide a way to let JS know when we need to rebuild files, which I think would allow for only rebuilding when necessary — both the JS and the rust files.

(But I don't know this well, I'm very green in JS)

[max-sixty]

That would be bad. Would you suggest a small change I could make to a rust file that should cause a rebuild?

If we cause a syntax error in bindings/prql-js/src/lib.rs then the playground should fail to build...

[richb-hanover]

On reconsideration, I deleted my earlier comment and am replacing with this...

... changing the rust code and running task run-playground will not cause a rebuild...

I think I understand your concern, which also is part of the reason that the npm install... has the --install-links=false option.

It sounds as if this is the desired workflow:

• Edit a rust file
• Run a task to compile
• task run-playground to try out the result. That must pick up a changed version of the rust compiler...

If that's the case, it seems that the install-playground-npm-dependencies task could also look at the rust compiler build product to decide when to run the full npm install...

1. Am I on the right track?
2. Would you suggest a rust file to change, and the task to run to re-build it?
3. Any thoughts about the build product from Rust that would be a good trigger?

Thanks.

[max-sixty]

• Edit a rust file

• Run a task to compile

• task run-playground to try out the result. That must pick up a changed version of the rust compiler...

I don't think that folks will expect two commands after editing a rust file. task run-playground should run the current code, regardless of when it was previously run. Currently this is done by the playground build calling into the prql-js build.

So either we need to either a) give that task knowledge of all the files it should check (probably crate/ and bindings/...), or b) have the JS build step know about what files it should check.

I think the latter would be great but would require redesigning the JS build with something like webpack.

The former is possible — if it provides a great speed-up then we could add it in. If it's only marginal, I would vote against, since if paths change and those become incorrect, it could be quite a confusing development experience.

[richb-hanover]

I see your concern and need to understand your workflow better, but don't have time now. I'm marking this a draft and will come back next week.

[max-sixty]

I've thought about this some more. Can we go over the motivating case for the main part of this code? (TBC, some of this is just better than the old code, so keen on it overall)

Currently when developing the playground

• If we change the prql compiler code, we need to re-run npm install, which then rebuilds prql-js. This took 59s on my machine (though I have prql-compiler dependencies cached), so isn't fast, but is tolerable.
• If we only want to change playground code, then we get hot reloading after running npm start. This took 9s to start on my machine, and then immediately reloads on any JS change. It does not pick up any prql compiler code changes.

I'm very up for speeding up dev loops. But what's the thing we're trying to speed up here — the first of these? How often are we running that?

[richb-hanover]

I have been thinking about workflows as well. I came to this PR to speed the second of your points: I only work on the Book or the Playground (or website, but I haven't touched that in a long time.)

This PR resolves my personal use case: run-book, run-playground, run-website now check to see if npm dependencies have changed and only run npm install when required. Changes to the pages of the Book/Playground/Web source don't require this since they rely on hot reloading, etc.

But it doesn't address your concern: making changes to the prql compiler code and then trying the changes in the Playground.

I don't think that folks will expect two commands after editing a rust file.

Never having worked on the rust code, I need to ask a naive question: What command(s) do you use with the current Taskfile.yml to edit a rust file and check it in the Playground?

I'm wondering if there's some condition that could be added to the install-playground-dependencies task (in this PR). It currently uses sources/generates to check the package.json, but perhaps it could also look at the date of the prql compiler binary. Thanks.

[max-sixty]

I have been thinking about workflows as well. I came to this PR to speed the second of your points: I only work on the Book or the Playground (or website, but I haven't touched that in a long time.)

This PR resolves my personal use case: [run-book], run-playground, run-website now check to see if npm dependencies have changed and only run npm install when required. Changes to the pages of the Book/Playground/Web source don't require this since they rely on hot reloading, etc.

OK, so the current commands work OK for you, it's just that the structure could use a refactor — is that right?

---

Never having worked on the rust code, I need to ask a naive question: What command(s) do you use with the current Taskfile.yml to edit a rust file and check it in the Playground?

task run-playground!

---

I'm wondering if there's some condition that could be added to the install-playground-dependencies task (in this PR). It currently uses sources/generates to check the package.json, but perhaps it could also look at the date of the prql compiler binary. Thanks.

That would be this option, quoting from above:

a) give that task knowledge of all the files it should check (probably crate/ and bindings/...),

[richb-hanover]

Slowly, slowly my knowledge about the project expands... You said:

... we need to re-run npm install, which then rebuilds prql-js ...

I think you mean that you edit the rust code, then compile it, and then need to run npm install to incorporate that new prql compiler binary (prql-js?) into the Playground. True?

If so, what would the install-playground-dependencies task use for the sources and generates section? How do crate/ and bindings/ fit in? And how would I test that they were working? (You suggested creating a syntax error in a rust file, but then... what command(s) would I use to trigger the rebuild?)

Thanks for your help

[max-sixty]

I think you mean that you edit the rust code, then compile it, and then need to run npm install to incorporate that new prql compiler binary (prql-js?) into the Playground. True?

Yes, thanks for being more explicit than me!

If so, what would the install-playground-dependencies task use for the sources and generates section? How do crate/ and bindings/ fit in? And how would I test that they were working? (You suggested creating a syntax error in a rust file, but then... what command(s) would I use to trigger the rebuild?)

The crates/ & bindings/ paths (from the root of the repo) could be sources

Then make a change like adding asdf in https://github.com/PRQL/prql/blob/9c6ea2a2fa3eea2597b8596c4231f46036a30f4b/crates/prql-compiler/src/lib.rs, and run task run-playground, and it should raise an error. Currently, running npm install from web/playground will pick this up (which is the command called by task run-playground). Does that make sense?

[richb-hanover]

Ahah! I understand now. (I had not realized that the wasm rebuild was part of the npm install - that output got scrolled off the screen by subsequent steps...)

And in fact asdf is a very potent poison in the lib.rs file. I see a path now. Thanks again.

[richb-hanover]

@max-sixty I also see the new commit that removes --install-links from the npm install... command. Is that OK now? And should I remove it in my branch? Thanks again

[max-sixty]

@max-sixty I also see the new commit that removes --install-links from the npm install... command. Is that OK now? And should I remove it in my branch? Thanks again

Yes you can always merge main in to yours

[richb-hanover]

I think I have it. Any changes to package.json or to rust files in prql/crates or prql/bindings will trigger a full npm install (without the --install-links option) when you execute task run-playground.

It remains true that the run-playground, run-book, or run-website tasks only run npm install if any of the above have changed. This speeds development of the "web" products a great deal.

[richb-hanover]

Actually, I missed out the changes for the "check brew dependencies" - I will add those back tomorrow morning

[max-sixty]

Great! Almost there...

[max-sixty]

Can we just combine this with the previous? Passing --force can override the sources check if we ever want to force an install

[richb-hanover]

I think I'd like to leave it like this. I factored it out when --install-links=false was necessary (only wanted to have that magic invocation in one place).

[max-sixty]

What's the difference between calling one vs the other?

[richb-hanover]

I don't understand the thrust of this question: "combine with the previous"? Which two are you suggesting could be combined?

Here's my understanding of the situation: In the original Taskfile.yaml there were several invocations of npm install --install-links=false which I factored into a single install-playground-dependencies-all task so the magic --install-links... wouldn't be screwed up in one of the multiple places. I also described that option in the comments (again, in one place). Additionally, I changed all the places that called npm install... to use the install-playground-dependencies task so they could take advantage of caching prior results.

Now that the --install-links=false is no longer required, we certainly could get rid of the (single) call to install-playground-dependencies-all and replace it with a simple npm install

But I'll also point out the "documentation advantage" of having a named task that unconditionally runs the proper npm install command in the case that the logic behind the install-playground-dependences gets screwed up. Instead of needing to remember where the --force option might be used, people can just task install-playground-dependencies-all to un-screw it.

I hope that addresses your question

[max-sixty]

What's the difference between install-playground-dependencies-all and install-playground-dependencies? I think one just checks the sources. So can we combine them?

[max-sixty]

Can we combine these though? I don't see why we have a task that seems to just wrap another task?

[max-sixty]

I tested this locally and it works well!

[max-sixty]

I didn't want to delay the excellent changes, and I didn't think I was being clear on one point — so merging and will open a quick PR for your review!