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

Migrate From Nx to Turborepo #33079

Merged
merged 23 commits into from
Jun 9, 2022
Merged

Migrate From Nx to Turborepo #33079

merged 23 commits into from
Jun 9, 2022

Conversation

ObliviousHarmony
Copy link
Contributor

@ObliviousHarmony ObliviousHarmony commented May 17, 2022

All Submissions:

Changes proposed in this Pull Request:

This pull request replaces Nx with Turborepo. This pull request comes with a number of tooling and documentation updates to make sure that everything is clear. I plan to publish internal and external communication prior to and alongside this change to minimize the impact it may have on ongoing work.

Here is a mostly exhaustive list of the changes proposed in this pull request:

  • Removal of Nx commands entirely. This means that commands like pnpm nx composer-install are no longer present in favor of running package manager commands directly. Note that in the case of Composer, each relevant package has a "postinstall" script that triggers a composer install automatically.
  • Standardized Test Scripts
    • test:e2e -> e2e
    • test:e2e-debug -> e2e:debug
    • test:e2e-dev -> e2e:dev
    • test:unit -> `test
  • Turborepo has been added as the executor of build, lint, test, and e2e commands. Use pnpm exec turbo run {command} --filter={package} to perform these actions. The output of the commands are cached where appropriate to minimize unnecessary executions. For example, pnpm nx build woocommerce is now pnpm exec turbo run build --filter=woocommerce.
  • For other commands, use pnpm's filter option to run them in the correct package. pnpm changelog --filter=woocommerce will run the "changelog" script from WooCommerce's packge.json.
  • woocommerce-admin is now referred to as @woocommerce/admin-library since the filtering syntax uses the package name.

Closes #32655

How to test the changes in this Pull Request:

  1. Run pnpm install
  2. Try out build, lint, test, and e2e commands.
  3. Make sure CI passes now that it was changed from Nx as well.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?
  • Have you created a changelog file for each project being changed, ie pnpm nx changelog <project>?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@github-actions github-actions bot added focus: react admin package: @woocommerce/e2e-environment Issues related to @woocommerce/e2e-environment package. package: @woocommerce/api Issues related to @woocommerce/api package. package: @woocommerce/e2e-core-tests Issues related to @woocommerce/e2e-core-tests package. package: @woocommerce/e2e-utils Issues related to @woocommerce/e2e-utils package. package: @woocommerce/api-core-tests Issues related to @woocommerce/api-core-tests package. package: @woocommerce/admin-e2e-tests issues related to @woocommerce/admin-e2e-tests package: @woocommerce/components issues related to @woocommerce/components package: @woocommerce/csv-export issues related to @woocommerce/csv-export package: @woocommerce/currency issues related to @woocommerce/currency package: @woocommerce/customer-effort-score issues related to @woocommerce/customer-effort-score package: @woocommerce/data issues related to @woocommerce/data package: @woocommerce/date issues related to @woocommerce/date package: @woocommerce/eslint-plugin issues related to @woocommerce/eslint-plugin package: @woocommerce/experimental issues related to @woocommerce/experimental package: @woocommerce/explat issues related to @woocommerce/explat package: @woocommerce/navigation issues related to @woocommerce/navigation package: @woocommerce/number issues related to @woocommerce/number package: @woocommerce/onboarding issues related to @woocommerce/onboarding package: @woocommerce/tracks issues related to @woocommerce/tracks package: dependency-extraction-webpack-plugin issues related to @woocommerce/dependency-extraction-webpack-plugin plugin: woocommerce Issues related to the WooCommerce Core plugin. labels May 17, 2022
@botwoo
Copy link
Collaborator

botwoo commented May 17, 2022

📊 Test reports for this pull request have been published and are accessible through the following links:

Latest commit referenced in the reports: Added woocommerce-legacy-assets Scripts 35c3848
This comment will automatically be updated with the latest referenced commit when you push new changes to this pull request.


Visit the WooCommerce Test Reports homepage to view all published reports. See the FAQs page if you're having problems accessing them.

@ObliviousHarmony
Copy link
Contributor Author

Interesting @psealock, I can't seem to reproduce this locally. What version of Composer are you using? It may also be worth noting that pnpm changelog will trigger the changelogger utility, not necessarily the add command. You can use pnpm changelog add --filter=woocommerce to add a changelog entry.

@psealock
Copy link
Contributor

psealock commented Jun 3, 2022

What version of Composer are you using?

Thats the issue, all working as it should. Thanks

Copy link
Member

@chihsuan chihsuan left a comment

Choose a reason for hiding this comment

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

@ObliviousHarmony Thanks for working on this!

I found pnpm exec turbo build --filter=woocommerce is not working for me. And looks like pnpm exec turbo run lint --filter=@woocommerce/admin-library is broken. 🤔

 pnpm exec turbo run lint --filter=@woocommerce/admin-library
• Packages in scope: @woocommerce/admin-library
• Running lint in 1 packages
@woocommerce/admin-library:lint: cache bypass, force executing 9d0a1ee78e1a1cf2
@woocommerce/admin-library:lint:
@woocommerce/admin-library:lint: > @woocommerce/admin-library@3.3.0 lint /Users/chihsuan/Projects/woocommerce/plugins/woocommerce-admin
@woocommerce/admin-library:lint: > pnpm run lint:js && pnpm run lint:css
@woocommerce/admin-library:lint:
@woocommerce/admin-library:lint:
@woocommerce/admin-library:lint: > @woocommerce/admin-library@3.3.0 lint:js /Users/chihsuan/Projects/woocommerce/plugins/woocommerce-admin
@woocommerce/admin-library:lint: > wp-scripts lint-js ./client --ext=js,ts,tsx
@woocommerce/admin-library:lint:
@woocommerce/admin-library:lint: WARNING: No configurations found in configuration directory:/Users/chihsuan/Projects/woocommerce/plugins/woocommerce-admin/config
@woocommerce/admin-library:lint: WARNING: To disable this warning set SUPPRESS_NO_CONFIG_WARNING in the environment.
@woocommerce/admin-library:lint: Error: ESLint configuration in plugin:@woocommerce/eslint-plugin/recommended is invalid:
@woocommerce/admin-library:lint: 	- Unexpected top-level property "overrides[0].extends".
@woocommerce/admin-library:lint:
@woocommerce/admin-library:lint: Referenced from: /Users/chihsuan/Projects/woocommerce/plugins/woocommerce-admin/.eslintrc.js
@woocommerce/admin-library:lint:     at validateConfigSchema (/Users/chihsuan/Projects/woocommerce/node_modules/.pnpm/eslint@5.16.0/node_modules/eslint/lib/config/config-validator.js:235:15)
@woocommerce/admin-library:lint:     at Object.validate (/Users/chihsuan/Projects/woocommerce/node_modules/.pnpm/eslint@5.16.0/node_modules/eslint/lib/config/config-validator.js:261:5)
@woocommerce/admin-library:lint:     at loadFromDisk (/Users/chihsuan/Projects/woocommerce/node_modules/.pnpm/eslint@5.16.0/node_modules/eslint/lib/config/config-file.js:544:19)
@woocommerce/admin-library:lint:     at load (/Users/chihsuan/Projects/woocommerce/node_modules/.pnpm/eslint@5.16.0/node_modules/eslint/lib/config/config-file.js:587:20)
@woocommerce/admin-library:lint:     at /Users/chihsuan/Projects/woocommerce/node_modules/.pnpm/eslint@5.16.0/node_modules/eslint/lib/config/config-file.js:453:36
@woocommerce/admin-library:lint:     at Array.reduceRight (<anonymous>)
@woocommerce/admin-library:lint:     at applyExtends (/Users/chihsuan/Projects/woocommerce/node_modules/.pnpm/eslint@5.16.0/node_modules/eslint/lib/config/config-file.js:431:26)
@woocommerce/admin-library:lint:     at loadFromDisk (/Users/chihsuan/Projects/woocommerce/node_modules/.pnpm/eslint@5.16.0/node_modules/eslint/lib/config/config-file.js:551:22)
@woocommerce/admin-library:lint:     at Object.load (/Users/chihsuan/Projects/woocommerce/node_modules/.pnpm/eslint@5.16.0/node_modules/eslint/lib/config/config-file.js:587:20)
@woocommerce/admin-library:lint:     at Config.getLocalConfigHierarchy (/Users/chihsuan/Projects/woocommerce/node_modules/.pnpm/eslint@5.16.0/node_modules/eslint/lib/config.js:240:44)
@woocommerce/admin-library:lint:  ELIFECYCLE  Command failed with exit code 2.
@woocommerce/admin-library:lint:  ELIFECYCLE  Command failed with exit code 1.
@woocommerce/admin-library:lint: Error: command finished with error: command (plugins/woocommerce-admin) pnpm run lint exited (1)
command (plugins/woocommerce-admin) pnpm run lint exited (1)

 Tasks:    0 successful, 1 total
Cached:    0 cached, 1 total
  Time:    1.167s

The other looks great! 👍

✅ Build pnpm exec turbo run build --filter=woocommerce
✅ Test pnpm exec turbo run test --filter=@woocommerce/admin-library
✅ Changelog pnpm changelog --filter=@woocommerce/components add
✅ E2E pnpm exec turbo run e2e --filter=woocommerce

plugins/woocommerce/bin/build-zip.sh Outdated Show resolved Hide resolved
plugins/woocommerce/tests/e2e/README.md Outdated Show resolved Hide resolved
plugins/woocommerce/includes/class-woocommerce.php Outdated Show resolved Hide resolved
@ObliviousHarmony
Copy link
Contributor Author

Thanks @chihsuan, could you share the error log for the WooCommerce build failure too?

@ObliviousHarmony
Copy link
Contributor Author

@chihsuan Oh, I see. You need the run part for Turbo to work 😃 I'll look at the linting issues tomorrow!

ObliviousHarmony and others added 3 commits June 6, 2022 12:43
Co-authored-by: Chi-Hsuan Huang <chihsuan.tw@gmail.com>
Co-authored-by: Chi-Hsuan Huang <chihsuan.tw@gmail.com>
@ObliviousHarmony
Copy link
Contributor Author

Are you using Node v16 @chihsuan? There are some dependencies that require it. I can't seem to replicate this error locally. If you are using a different version, I would suggest grabbing NVM. Our repository has a .nvmrc file that will set the correct version of node when you run nvm use.

@chihsuan
Copy link
Member

chihsuan commented Jun 7, 2022

Are you using Node v16 @chihsuan? There are some dependencies that require it. I can't seem to replicate this error locally. If you are using a different version, I would suggest grabbing NVM. Our repository has a .nvmrc file that will set the correct version of node when you run nvm use.

Yea, I'm using Node v16. Let me try again. 🤔

@chihsuan
Copy link
Member

chihsuan commented Jun 7, 2022

@ObliviousHarmony I just cleaned all the dist folders and it’s working for me now! Sorry for the false positive.

@ObliviousHarmony ObliviousHarmony marked this pull request as ready for review June 9, 2022 17:52
These were previously in Nx executors. They need
to be in a script so that Turbo can run them.
Copy link
Contributor

@jonathansadowski jonathansadowski left a comment

Choose a reason for hiding this comment

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

👍 Nice work. Look forward to this being merged.

@ObliviousHarmony ObliviousHarmony merged commit b793140 into trunk Jun 9, 2022
@ObliviousHarmony ObliviousHarmony deleted the try/turborepo branch June 9, 2022 21:40
@github-actions github-actions bot added this to the 6.7.0 milestone Jun 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2022

Hi @ObliviousHarmony, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

  • Add the release: add testing instructions label

@leerob
Copy link

leerob commented Jun 14, 2022

This is awesome! Nice work 👏

@erkand-imeri
Copy link

erkand-imeri commented Jun 16, 2022

Hello @ObliviousHarmony, I was looking into monorepo tools like nx and Turborepo recently. And would apreciate if you can summarize why did you guys decide to switch from nx to turborepo?

Best regards, Erkand.

@ObliviousHarmony
Copy link
Contributor Author

Howdy @erkand-imeri, before we begin, I want to be very clear: Nx is a great tool. While it didn't work out for our use case, I don't think that makes it worse. A lot of this comes down to our own workflows and differences between some of our processes and the strong opinions of Nx. With that said, I'll share an explanation from an internal document:


Ultimately it a came down to a few things:

  • Nx favors a single-version policy that wants all of your dependencies in a top-level package.json file. Unless you conform to this structure you can’t use any of the plugins. Since plugins are a big part of Nx’s value, not being able to use them undercuts that.
  • Nx mangles arguments. Rather than being able to pass arguments through to the task from Nx, it requires that you build a custom executor that wraps the command and defines all of the available properties. The fact that a maintainer asked for a reproduction case and never replied to the issue again is not a good sign to me. In the migration issue in our repository, a maintainer replied that they’re currently fixing that but the lack of communication with their own community is still not a good sign.
  • We have no need for the Nx Cloud caching functionality.
  • Since we’re running package.json scripts, all of the project.json files are redundant.

With all of these considerations, we had a few choices:

  • Commit to a single-version policy for all of our packages. This might be desirable in some cases, but, there are tools we can use to implement that in the cases we want to. Claudio looked at some and found syncpack which we might want to adopt.
  • Write custom executors for all of our tools that run against local dependencies instead of repository-wide dependencies. This seems like a lot of work and unwieldy to maintain.

Even at best, then, most of the unique value provided by Nx is lost. We are essentially using it as a package.json task runner with support for caching. In the face of that, I wanted to look at some alternatives. Given that we only really need a package.json task runner with support for caching, Turborepo seemed like a strong choice.

  • Configurable Caching: Unlike Nx, which has an opinionated caching system, Turbo allows you to define all of the inputs to watch for changes. You get to decide what files should trigger a rebuild, unlike Nx, which seems to decide when to rebuild based on ambiguous criteria.
  • PNPM Style --filter Syntax: Since our repository uses pnpm, it’s very convenient to have a flag that serves the same purpose in both tools. There’s only one syntax to learn!

There is a comparison page on Nx's website looking at Turborepo. This is dated December 14th, 2021 and was likely written in response to Vercel’s acquisition of Turborepo. I will take a look at each of the major points and discuss their impact on the decision to migrate.

  • Understanding your workspace: On the point about Nx analyzing source code, this isn’t true if you don’t use plugins. Since we can’t use them, it doesn’t really matter to us. It's true Turborepo has no visibility rules, but, I’m not really sure that matters. The point of a dependency is to depend on it, right? Maybe this is a problem at the scale of someone like Google with tons of packages?
  • Project graph visualization: This is still true, but, I question how much it really matters. It might be useful when trying to trace the dependencies of a single project, but, I’m not sure we need a comprehensive client-side app.
  • Local task coordination: Like with other things, it’s true Turbo doesn’t support plugins.
  • Local computation caching: For the cache restorations, there’s a pull request that has received some pretty heavy attention from the maintainers for the last month. This difference will be removed sooner rather than later. As for the terminal output, this one is true, but, it feels like it should be a relatively easy upstream change based on the issue and what I’ve seen of the internals so far.
  • Distributed computation caching: You are now able to implement your own computation cache for Turborepo and configure it to use it instead of Vercel’s service offering.
  • Distributed task execution: This is true, Turborepo does not support it. I’m not sure this matters at the scale we’re operating though.
  • Editor support: This is also true, but, I think most of use the CLI for everything anyway.
  • Transparency: This bit is true, it does transform the output. There’s a WIP PR aimed at removing the prefixing though, which would make the input the same (outside of the colors).
  • Plugins and Supporting Features: As mentioned a few times, we aren’t using plugins or the cloud cache.
  • Tech and Performance: In that animation, it takes 4.15 seconds for Turborepo to run against 5 packages. In our Monorepo, it runs against 22 and took 597ms just now. When I was making the transition I noticed that an overly-permissive pnpm-workspace.yaml file could make the startup take a while. It does a glob walk of directories, so things like packages/** will recursively search through every directory to try and find a workspace package. When I changed this to the more specific packages/js/* it sped up instantly. I’m not sure about the cache restoration performance, but as noted above, they’re adding detection to avoid doing that if it isn’t necessary.
  • Community: Turborepo does have a smaller community than Nx but I don’t know if that will always be the case, nor is it necessarily an argument that it’s better. As for the license, I’m not sure about whether or not the differences between MIT and MPL 2.0 matter much for a tool. The only difference is that you could build your own Nx and distribute it as proprietary work while with Turborepo you’d have to contribute those changes back (unless you isolated the Turborepo code).

With all of that said, it felt like the right decision to move to Turborepo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: dependency-extraction-webpack-plugin issues related to @woocommerce/dependency-extraction-webpack-plugin package: @woocommerce/admin-e2e-tests issues related to @woocommerce/admin-e2e-tests package: @woocommerce/api Issues related to @woocommerce/api package. package: @woocommerce/api-core-tests Issues related to @woocommerce/api-core-tests package. package: @woocommerce/components issues related to @woocommerce/components package: @woocommerce/csv-export issues related to @woocommerce/csv-export package: @woocommerce/currency issues related to @woocommerce/currency package: @woocommerce/customer-effort-score issues related to @woocommerce/customer-effort-score package: @woocommerce/data issues related to @woocommerce/data package: @woocommerce/date issues related to @woocommerce/date package: @woocommerce/e2e-core-tests Issues related to @woocommerce/e2e-core-tests package. package: @woocommerce/e2e-environment Issues related to @woocommerce/e2e-environment package. package: @woocommerce/e2e-utils Issues related to @woocommerce/e2e-utils package. package: @woocommerce/eslint-plugin issues related to @woocommerce/eslint-plugin package: @woocommerce/experimental issues related to @woocommerce/experimental package: @woocommerce/explat issues related to @woocommerce/explat package: @woocommerce/navigation issues related to @woocommerce/navigation package: @woocommerce/number issues related to @woocommerce/number package: @woocommerce/onboarding issues related to @woocommerce/onboarding package: @woocommerce/tracks issues related to @woocommerce/tracks plugin: woocommerce Issues related to the WooCommerce Core plugin.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace nx With turbo
7 participants