privatenumber / esbuild-loader

💠 Speed up your Webpack with esbuild ⚡️
MIT License
3.58k stars 106 forks source link

fix: `tsconfig.json` edge-cases in dependencies #377

Closed glsignal closed 3 months ago

glsignal commented 3 months ago

Fixes #363 where esbuild-loader would resolve the tsconfig.json of external files under certain conditions, which most recently became a problem when one of these configs included an extends option that referenced a package not in the project's dependency tree, making it impossible to resolve.

The patch changes this behaviour and will attempt to resolve the tsconfig.json still, however when it encounters a problem, if the resource associated with the tsconfig is a third party dependency, it will suppress the error and log a warning instead of bailing.

privatenumber commented 3 months ago

Thanks for the PR!

RE: fix(dev): prevent fs-fixture causing a re-run loop

Can you provide env details? (OS, tooling versions, etc) I haven't needed this before.

glsignal commented 3 months ago

Sure thing!

OS: Linux lovelace 6.9.7-arch1-1 #1 SMP PREEMPT_DYNAMIC Fri, 28 Jun 2024 04:32:50 +0000 x86_64 GNU/Linux Node: v20.14.0 pnpm: 9.2.0 (via corepack)

It might well be a linux thing (happy to rebase without that commit), but here's what the problem looked like for me in case it gives any additional insight

image

glsignal commented 3 months ago

I removed the commit that introduced the --ignore /tmp since the issue (if it's an issue) doesn't seem to be related to this project/PR, and have set up a reproduction in https://github.com/glsignal/fs-fixture-tsx-watch-loop (edit: and it's only looping on my linux machines, seems fine in macos)

privatenumber commented 3 months ago

Sorry about the linting/type-check errors. I fixed them in the master branch directly.

It looks like this PR no longer compiles TS in node_modules, but we actually still want to compile them. To do this, we'll probably try to read the tsconfig.json file, but we should ignore any tsconfig errors in node_modules.

glsignal commented 3 months ago

It looks like this PR no longer compiles TS in node_modules, but we actually still want to compile them. To do this, we'll probably try to read the tsconfig.json file, but we should ignore any tsconfig errors in node_modules.

Thanks! I'd completely changed the behaviour without realising by using that early return statement.

I've pushed a change which should still try to apply the transform and log a warning if the tsconfig fails to load for some reason. In short, it shouldn't change any behaviour except for when the tsconfig fails to resolve specifically for resources somewhere in node_modules

Can you make this a .ts file to confirm TS files in node_modules can still be compiled?

Where I'm running into trouble is putting together the test case with specifically .ts source in the package, any path that points to .ts source fails to resolve (Module not found: Error: Can't resolve './testFn' etc.)

I must admit this is an area where I'm not as knowledgeable about the inner workings of module resolution to have a good idea of how to write the appropriate setup for it, do you have any advice here?

Never mind that, they're now .ts sources in the test

glsignal commented 3 months ago

(fyi; squashed/rebased for a tidier history - no other changes)

privatenumber commented 3 months ago

I made several changes:

privatenumber commented 3 months ago

:tada: This issue has been resolved in version 4.2.2 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

privatenumber commented 3 months ago

Thanks for the PR and collaboration @glsignal! Appreciate your contribution 🙏

glsignal commented 3 months ago

Thank you for the support and pushing it over the finish line @privatenumber! Great collab 😄