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

Upgrade esbuild-loader and @craco/craco #73

Merged
merged 7 commits into from
Mar 12, 2024

Conversation

Crecket
Copy link
Contributor

@Crecket Crecket commented Jan 29, 2024

Versions were way behind causing issues for our project, alle the examples seem to be working using these versions.

Modified the EsbuildPlugin import as mentioned in the esbuild-loader migration docs and removed the typescript check as the esbuild-loader now does that automatically (See 3.0.0)

Added a requirement for node 16 matching esbuild-loader requirements v3

@pradel
Copy link
Owner

pradel commented Jan 29, 2024

@Crecket thanks for the pr, it seems that the tests are broken with the new changes, can you please have a look?

for the tests, we should probably stop supporting node 16 as it's not LTS anymore and move the ci to run on node 18 + 20

@Crecket
Copy link
Contributor Author

Crecket commented Jan 29, 2024

Removing the loader means it will check for jsx/tsx file extensions to decide which one to use and it was failing on JSX content without a .jsx extension in the example. It does seem nice to let the esbuild-loader handle that behavior over this plugin checking for a tsconfig but I guess we could revert that change and not touch the old behavior as I imagine this would break existing apps and force people to use .jsx over regular .js extensions in their projects

Changed the default node version to 20 for the release step / swapped to running only node 16/18/20 for the tests

Besides that, I upgraded some other dependencies to fix the builds and added a missing babel dependency that was breaking the typescript example for some reason.

@@ -9,15 +9,15 @@
"src"
],
"peerDependencies": {
"@craco/craco": "^6.0.0 || ^7.0.0 || ^7.0.0-alpha",
"@craco/craco": "^^7.0.0 || ^7.0.0-alpha",
Copy link
Owner

Choose a reason for hiding this comment

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

let's remove the alpha option there, if people use the latest version they should upgrade to craco stable

},
"devDependencies": {
"@craco/craco": "6.4.3",
"@craco/craco": "7.0.0",
Copy link
Owner

Choose a reason for hiding this comment

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

let's use the latest craco version here?

@pradel
Copy link
Owner

pradel commented Jan 30, 2024

@Crecket I think the tests are failing because you are using pnpm 8+ and the ci is using pnpm 6 so the generated lockfile is incompatible.

@yaronya
Copy link
Contributor

yaronya commented Feb 21, 2024

Any chance we can get this in soon? I was looking to open a PR but found that it's something that's in the doing already.

I'd also suggest to keep esbuild-loader as a peer dependency - it makes it easier for users of this plugin to control the version they wish to use

@Crecket
Copy link
Contributor Author

Crecket commented Feb 23, 2024

I'll look into this again this weekend, been away/busy for a bit so haven't had time to fix the last few issues yet

@tomer-landesman
Copy link

I see CI is passing now, any chance it can get approved? @pradel

@pradel pradel merged commit 935c2f9 into pradel:main Mar 12, 2024
4 checks passed
@github-actions github-actions bot mentioned this pull request Mar 12, 2024
@pradel
Copy link
Owner

pradel commented Mar 12, 2024

Published in 0.6.0

@Crecket Crecket deleted the upgrade-craco-and-esbuild-loader branch March 12, 2024 12:27
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.

4 participants