-
Notifications
You must be signed in to change notification settings - Fork 357
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
Conversation
@linode/validation
and @linode/api-v4
validation
and api-v4
validation
and api-v4
tsup
for validation
and api-v4
Awesome work Banks, this is super interesting! I have some observations and a bunch of thoughts/questions, forgive me for the wall of text:
|
@jdamore-linode Thank you for all of that feedback!
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. |
@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! |
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.
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
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.
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": "*", |
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 you explain this change a bit? Would this allow us to eliminate part of a step from our release process?
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 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.
replaced by #8484 |
Description 📝
@linode/api-v4
and@linode/validation
@linode/validation
and@linode/api-v4
#8424 and Use esbuild for@linode/api-v4
and@linode/validation
#8309Main Changes
esm
,commonjs
, andiife
format, no longerumd
onlyesm
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>
)index.js
file for each format and oneindex.d.ts
file@linode/validation
live reloads@linode/api-v4
), we no longer support imports from a subdirectory like (@linode/api-v4/lib/account
)@swc/jest
to run@linode/api-v4
's unit tests@linode/api-v4
and@linode/validation
@linode/validation
because we don't write unit tests for that package@linode/api-v4
's jest config to thepackage.json
instead of a dedicated jest config fileyarn up
still exists for Cypress butyarn 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 beforePros of tsup
Cons of tsup
How to test 🧪
yarn
to install packagesyarn build:validation && yarn build:sdk
so you have dependencies builtyarn dev
Looking forward
ts-jest
with a new jest loader, resulting in much faster unit test run timesWebpack
withVite