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

M3-5873: Use tsup for validation and api-v4 #8447

Closed
wants to merge 17 commits into from

Conversation

bnussman
Copy link
Member

@bnussman bnussman commented Jul 5, 2022

Description 📝

Main Changes

  • We now build esm, commonjs, and iife format, no longer umd only
    • esm for modern web (dependencies like axios not bundled, Cloud Manager should use this)
    • commonjs for Node.js (dependencies like axios not bundled)
    • iife for browsers (imported dependencies bundled so you can use this directly in the browser with a <script>)
  • We now bundle into one index.js file for each format and one index.d.ts file
    • Previously we transpiled every single file into its own js file and generated a types file for every single file then made a bundled index.js in the root directory
  • @linode/validation live reloads
  • You must import from the root only (@linode/api-v4), we no longer support imports from a subdirectory like (@linode/api-v4/lib/account)
  • Use @swc/jest to run @linode/api-v4's unit tests
  • Remove webpack and babel dependencies from @linode/api-v4 and @linode/validation
  • Remove jest and related testing code from @linode/validation because we don't write unit tests for that package
  • Moved @linode/api-v4's jest config to the package.json instead of a dedicated jest config file
  • yarn up still exists for Cypress but yarn dev should be preferred for most of our daily development because it will start up quicker assuming @linode/api-v4 and @linode/validation have been built at sometime before

Pros of tsup

  • A wrapper over esbuild to makes things easy on us
  • Pretty fast 💨
  • Can emit single declaration file out of the box (we use a webpack plugin for this right now)
  • Sponsored by companies like Github
  • es5 support if we need it

Cons of tsup

How to test 🧪

  • Clear out old builds of api-v4 and validation so they can't be used by manager
rm -rf packages/api-v4/lib && rm -rf packages/validation/lib && rm packages/api-v4/index.js && rm packages/api-v4/index.d.ts && rm packages/validation/index.js && rm packages/validation/index.d.ts
  • Run yarn to install packages
  • Run yarn build:validation && yarn build:sdk so you have dependencies built
  • Run yarn dev

Looking forward

  • This change will allow us to replace ts-jest with a new jest loader, resulting in much faster unit test run times
  • This will also allow us to replace Webpack with Vite
    • This will result in much better development experience
      • Hot reloading of .env
      • Live reload without browser refresh
      • Zero dev server start up times

@bnussman bnussman self-assigned this Jul 5, 2022
@bnussman bnussman changed the title Use tsup for @linode/validation and @linode/api-v4 Use tsup for validation and api-v4 Jul 5, 2022
@bnussman bnussman changed the title Use tsup for validation and api-v4 M3-5873: Use tsup for validation and api-v4 Jul 6, 2022
@jdamore-linode
Copy link
Contributor

jdamore-linode commented Jul 14, 2022

Awesome work Banks, this is super interesting! I have some observations and a bunch of thoughts/questions, forgive me for the wall of text:

  1. With tsup, I'm no longer seeing the TypeScript d.ts output errors that I've been seeing for the past few months. (Context)
  2. I built api-v4 5 times each on develop and on this branch. On develop, the build time averages 15.24s. On this branch, it averages 4.61s. Nice!
    • Ex: seq 5 | xargs -I{} yarn build in the api-v4 directory
  3. I'm very pleased with yarn dev's speed, and even more excited that we're laying the groundwork to potentially switch to Vite in the future -- I've only heard great things about it
  4. If Cypress is the only thing making the yarn up command necessary, I'd be happy to work with you on setting up an alternative so we can potentially get rid of yarn up. I'm already using an alternative for my Cypress/Jenkins work, so this isn't really new territory.
    • And just to confirm, Cypress runs just fine when I use yarn dev instead of yarn up, so off the top of my head I think the only thing relying on yarn up would be the GitHub Actions files
  5. I'm a bit nervous about the import changes -- this is a breaking change, and even though we can technically get away with this being a minor version increment for @linode/api-v4 and @linode/validation since we're still pre-v1.0.0, I would expect this to break most projects that might rely on these packages and so it might be worth more discussion
    • Building off of this, are the import changes a necessity, or would it be possible to preserve the import {...} from '@linode/api-v4/lib/{...}' imports without nullifying the other benefits we get by using tsup?

@bnussman
Copy link
Member Author

bnussman commented Jul 14, 2022

@jdamore-linode Thank you for all of that feedback!

  • As for yarn up. I can leave it as-is here and we can update or remove it down the road as the e2es change
  • Regarding the import changes, I agree with everything you said. It's definitely unfortunate. There's a few ways we can try going about backwards compatibility with import {...} from '@linode/api-v4/lib/{...}' but it would possible require extra builds that end up being redundant. I tried experimenting with the exports field in the package.json of api-v4 to try to make lib imports resolve to the single index.ts file, but it confused typescript, but could be solvable by a typescript update.
  "exports": {
    ".": "./lib/esm/index.js",
    "./lib/*": "./lib/esm/index.js"
  },

Another idea in general would to be to scrap this PR and try to make a tsup PR that tries to exactly recreate our current way of building with the exact same output files and structure. That way we can keep imports all the same and just benefit from faster builds.

@jdamore-linode
Copy link
Contributor

@bnussman Sounds good to me! I don't think scrapping this PR is necessary but I would be interested in the rest of the team's thoughts once they have a chance to look at this PR. It's hard to say how many people besides us would even be impacted by the import change, and if I had to guess I'd assume it's very few!

I'm going to approve this even though it's a POC -- I'd be happy to re-review if you make any big changes!

Copy link
Contributor

@DevDW DevDW left a comment

Choose a reason for hiding this comment

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

Just noting that I've started looking through this. The app compiled and functionality hasn't been adversely impacted from what I've observed so far. Good speed improvements during the build process.

Going to continue testing and delve deeper into the configuration options to see if there are any tweaks we should make

Copy link
Contributor

@DevDW DevDW left a comment

Choose a reason for hiding this comment

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

With regards to the import changes, it'd be great if we could maintain backwards compatibility without significantly compromising the benefits of moving to tsup. If it's one or the other though, I would lean towards getting the benefits of tsup.

Once we merge this, we should let it cook in develop for a while before the following release to see if any issues arise

"dependencies": {
"@linode/validation": "0.12.0",
"@linode/validation": "*",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain this change a bit? Would this allow us to eliminate part of a step from our release process?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm having a hard time finding documentation on this, but from what I understand, yarn will use the correct local version because it is in the yarn workspace.

@bnussman
Copy link
Member Author

bnussman commented Aug 5, 2022

replaced by #8484

@bnussman bnussman closed this Aug 5, 2022
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.

3 participants