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

Use turbo for dev #383

Merged
merged 5 commits into from
Feb 16, 2022
Merged

Use turbo for dev #383

merged 5 commits into from
Feb 16, 2022

Conversation

jplhomer
Copy link
Contributor

@jplhomer jplhomer commented Dec 17, 2021

Description

  • IDK if Turbo is compatible with dev style commands, where a tsc --watch has to run, complete initial build, and then another consuming package can use it. I can't find any docs on this.
  • tl;dr is that dev#vite.config.js fails because @shopify/hydrogen/plugin does not yet resolve to a compiled file when it tries to run
  • Would be cool if we didn't need to compile hydrogen at all and instead just let Vite do the heavy lifting? That seems to be the approach the npx create-turbo scaffolded app takes: let Next.js transpile the UI deps 🤔

Update: Solved this by doing two things:

  1. Don't rm -rf the dist output as a predev task anymore. YOLO it's probably fine.
  2. Make @shopify/hydrogen#build a prereq for running dev in the pipeline. This means it's always available for the starter template Vite plugin.

Fixes #667

Additional context


Before submitting the PR, please make sure you do the following:

  • Read the Contributing Guidelines
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123)

@jplhomer jplhomer requested a review from cartogram December 17, 2021 20:51
@jplhomer jplhomer changed the title wip: try to use turbo for dev Use turbo for dev Feb 15, 2022
@jplhomer jplhomer marked this pull request as ready for review February 15, 2022 15:48
@jplhomer jplhomer requested a review from a team February 15, 2022 15:49
@@ -3,7 +3,7 @@
"private": true,
"version": "0.10.1",
"scripts": {
"dev": "vite",
"_dev": "vite",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Annoying, but we don't want to run the dev mode of this playground package during normal dev. Interested in future support for an ignore flag in Turbo. In the meantime, this is rarely used so I'm 💯 ok with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does --ignore give you this from the top-level script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it's broken right now vercel/turborepo#445 and will be replaced by a new --filter syntax eventually.

Copy link
Contributor

@frehner frehner left a comment

Choose a reason for hiding this comment

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

Questions, mostly because I haven't used Turbo before and wanted to understand it better. Thanks in advance

package.json Outdated
"build-hydrogen-template": "yarn workspace template-hydrogen-default build",
"build-lint": "yarn workspace eslint-plugin-hydrogen build",
"dev": "yarn turbo run dev --parallel --continue",
"build": "yarn turbo run build --parallel --continue",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just looking at the turbo docs here, so I understand:

--parallel mentions that it's good for "developing with live reloading." For a build script, does this come with some cons of using it?


--continue says that it will continue even in the presence of an error code. I don't think we would want that for a build script? (If something fails during a build, we would that to stop so that we can't publish a broken build or something, right?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah good catch - copy/pasted from dev without thinking of this.

package.json Outdated
"build-dev": "yarn build-hydrogen-template",
"build-hydrogen-template": "yarn workspace template-hydrogen-default build",
"build-lint": "yarn workspace eslint-plugin-hydrogen build",
"dev": "yarn turbo run dev --parallel --continue",
Copy link
Contributor

Choose a reason for hiding this comment

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

Trying to learn Turbo here, forgive my questions:

Should we consider adding --no-cache to the dev script? It says that it's "useful for watch commands"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've set cache: false in turbo.json, which I assume is the same thing.

@@ -3,7 +3,7 @@
"private": true,
"version": "0.10.1",
"scripts": {
"dev": "vite",
"_dev": "vite",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does --ignore give you this from the top-level script?

turbo.json Outdated
"$schema": "https://turborepo.org/schema.json",
"baseBranch": "origin/main",
"pipeline": {
"build": {},
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that setting outputs is important to the caching aspect of Turbo, I think? https://turborepo.org/docs/reference/configuration#outputs

So we would want to put dist in there, or am I misunderstanding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It defaults to [dist/**, build/**], so we should be good.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah missed that. 👍

@jplhomer jplhomer merged commit a0361eb into main Feb 16, 2022
@jplhomer jplhomer deleted the jl-turbooooooo branch February 16, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve local dev of monorepo
2 participants