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

build: update deps #2158

Merged
merged 31 commits into from
May 1, 2024
Merged

build: update deps #2158

merged 31 commits into from
May 1, 2024

Conversation

maschad
Copy link
Member

@maschad maschad commented Apr 24, 2024

I explained here the rationale for re-opening this.

This undoes the revert done here I chose to use the original branch as a revert of a revert would make the commit history look less readable.

I've ran these tests in the browser at least 4 times with no failures so I think it's safe to merge.

@maschad maschad marked this pull request as draft April 24, 2024 20:30
@maschad maschad self-assigned this Apr 24, 2024
@maschad maschad added the chore Issue is a chore label Apr 24, 2024
@maschad maschad marked this pull request as ready for review April 24, 2024 21:51
Dhaiwat10
Dhaiwat10 previously approved these changes Apr 24, 2024
nedsalk
nedsalk previously approved these changes Apr 25, 2024
arboleya
arboleya previously approved these changes Apr 25, 2024
@petertonysmith94
Copy link
Contributor

With the vite upgrade, they advise to change our vite.config.ts to vite.config.mts as vite@^5 is shipped as an ESM only package.

Do we have a list of the breaking changes for the other packages updated?

@maschad
Copy link
Member Author

maschad commented Apr 26, 2024

ESM only package

The vite.config.ts files we have already in ESM syntax so I don't think this is an issue, that recommendation is for troubleshooting non-ESM compatible imports.

@maschad
Copy link
Member Author

maschad commented Apr 29, 2024

@danielbate @Torres-ssf can you let me know if you are still experiencing performance issues on this? it should be fine now.

@maschad maschad mentioned this pull request Apr 29, 2024
Torres-ssf
Torres-ssf previously approved these changes Apr 30, 2024
@Torres-ssf
Copy link
Contributor

I had no issues running the tests using this branch

@petertonysmith94
Copy link
Contributor

Just merged master and got the error we experienced before the release.

FAIL packages/utils/src/utils/chunkAndPadBytes.test.ts [ packages/utils/src/utils/chunkAndPadBytes.test.ts ]
Error: No test suite found in file /home/runner/work/fuels-ts/fuels-ts/packages/utils/src/utils/chunkAndPadBytes.test.ts


There are also a lot of warnings within the browser tests which aren't visible on master.

Module "util" has been externalized for browser compatibility. Cannot access "util.debuglog" in client code. See https://vitejs.dev/guide/troubleshooting.html#module-externalized-for-browser-compatibility for more details.

@maschad
Copy link
Member Author

maschad commented Apr 30, 2024

Just merged master and got the error we experienced before the release.

FAIL packages/utils/src/utils/chunkAndPadBytes.test.ts [ packages/utils/src/utils/chunkAndPadBytes.test.ts ]
Error: No test suite found in file /home/runner/work/fuels-ts/fuels-ts/packages/utils/src/utils/chunkAndPadBytes.test.ts

There are also a lot of warnings within the browser tests which aren't visible on master.

Module "util" has been externalized for browser compatibility. Cannot access "util.debuglog" in client code. See vitejs.dev/guide/troubleshooting.html#module-externalized-for-browser-compatibility for more details.

Good catch @petertonysmith94 , it seems my previous lockfile was causing vitest to run v1.5 despite those deps not being updated to that vitest version, a bit strange considering that's outside the semver range, I must have forgotten to run pnpm install ... nevertheless I've resolved that and the warnings and flakey test are gone now 😄

@maschad maschad requested a review from Torres-ssf April 30, 2024 22:11
Copy link
Contributor

@petertonysmith94 petertonysmith94 left a comment

Choose a reason for hiding this comment

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

@maschad that is odd, potentially we need to add --frozen-lockfile to the pnpm install for our CI environment?

Either way, this looks good now 🥳

Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

Good one! 💪

I just tested this branch locally, and everything went well.

Please remember to double-check it after it goes to master. 🙂

@maschad maschad enabled auto-merge (squash) May 1, 2024 12:04
Copy link
Contributor

github-actions bot commented May 1, 2024

Coverage Report:

Lines Branches Functions Statements
79.26%(+0.01%) 69.26%(+0.05%) 77.36%(+0%) 79.38%(+0.01%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/account/src/providers/transaction-request/transaction-request.ts 88.57%
(+0%)
74.68%
(+1.35%)
91.83%
(+0%)
88.88%
(+0%)

@maschad maschad dismissed danielbate’s stale review May 1, 2024 14:11

The changes have been addressed

@maschad maschad merged commit 86543ed into master May 1, 2024
19 checks passed
@maschad maschad deleted the build/upgrade-deps branch May 1, 2024 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Issue is a chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants