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

glsignal · 2024-07-13T22:06:35Z

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 · 2024-07-13T22:50:24Z

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 · 2024-07-13T23:01:19Z

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

glsignal · 2024-07-14T09:46:41Z

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 · 2024-07-15T02:52:54Z

src/loader.ts

@@ -81,6 +81,12 @@ async function ESBuildLoader(
 		} else {
 			/* Detect tsconfig file */

+			// Don't look for tsconfig.json based on external sources (see
+			// https://github.com/privatenumber/esbuild-loader/issues/363)
+			if (resourcePath.match(/node_modules/) !== null) {


Suggested change

if (resourcePath.match(/node_modules/) !== null) {

if (resourcePath.includes('/node_modules/')) {

This could catch something like node_modules_file. I'm wondering if my suggestion would work on Windows where they use blackslashes... the tests will tell

Great point, thanks for catching this! In the latest commit, I changed the search a little and hopefully the path.sep tactic means it is a little more portable

privatenumber · 2024-07-15T02:54:54Z

tests/specs/tsconfig.ts

+					// Fake external dependency
+					node_modules: {
+						'fake-lib': {
+							'index.js': 'export function testFn() { return "Hi!" }',


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

privatenumber · 2024-07-15T02:56:24Z

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 · 2024-07-15T09:12:38Z

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 · 2024-07-16T09:07:34Z

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

glsignal · 2024-07-16T09:59:36Z

src/loader.ts

+					this.emitWarning(
+						new Error(`[esbuild-loader] Error while discovering tsconfig.json in dependency: "${error?.toString()}"`),
+					);


I tend to log more by default for cases such as these, but I'd defer to your preference as the maintainer of the project. Do you think there's value in logging this, or would it be preferable to silently ignore it?

For the original issue that sparked this PR, I don't think it would be a directly actionable warning but may provide a breadcrumb if someone is debugging an unusual output

I think it's a good idea. I would appreciate it as a user.

For the original issue, I think the warning should direct the users to report bugs to the appropriate repo.

I'll update the error message in get-tsconfig to be more specific about which file it's trying to resolve.

Original issue: privatenumber#363, esbuild-loader would attempt to resolve typescript configs for external sources under some conditions, and cause problems (in this case, the resolved tsconfig.json referenced a config that is not in the dependency tree and thus unresolvable) tsconfig fix: warn on tsconfig resolution error, continue transform

privatenumber · 2024-07-16T11:04:13Z

I made several changes:

Only search for a tsconfig.json if the file is in the local project, or if it's a dependency with a TS extension
Only apply provided tsconfig path to files in local project
Added 2 more tests to cover these cases

privatenumber · 2024-07-16T11:08:03Z

🎉 This issue has been resolved in version 4.2.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

privatenumber · 2024-07-16T11:09:19Z

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

glsignal · 2024-07-16T11:12:40Z

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

glsignal force-pushed the issue-363-tsconfig-resolution branch 2 times, most recently from b95fd2b to 6868a2e Compare July 14, 2024 09:43

privatenumber reviewed Jul 15, 2024

View reviewed changes

glsignal force-pushed the issue-363-tsconfig-resolution branch 2 times, most recently from c8c8668 to 14e606f Compare July 16, 2024 09:07

glsignal commented Jul 16, 2024

View reviewed changes

privatenumber force-pushed the issue-363-tsconfig-resolution branch from 14e606f to 5a31cd4 Compare July 16, 2024 10:13

privatenumber added 4 commits July 16, 2024 19:32

wip

cb3ad94

wip

0083a9b

wip

f8c6593

wip

6fb24a7

privatenumber changed the title ~~Bugfix: prevent esbuild-loader from attempting to resolve typescript configs for external sources~~ fix: tsconfig.json parsing edge-cases in dependencies Jul 16, 2024

privatenumber changed the title ~~fix: tsconfig.json parsing edge-cases in dependencies~~ fix: tsconfig.json edge-cases in dependencies Jul 16, 2024

privatenumber merged commit e252654 into privatenumber:master Jul 16, 2024
1 check passed

privatenumber added the released label Jul 16, 2024

glsignal deleted the issue-363-tsconfig-resolution branch July 16, 2024 11:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

glsignal commented Jul 13, 2024 •

edited

Loading

privatenumber commented Jul 13, 2024

glsignal commented Jul 13, 2024 •

edited

Loading

glsignal commented Jul 14, 2024 •

edited

Loading

privatenumber Jul 15, 2024 •

edited

Loading

glsignal Jul 15, 2024

privatenumber Jul 15, 2024

glsignal Jul 15, 2024

privatenumber commented Jul 15, 2024

glsignal commented Jul 15, 2024 •

edited

Loading

glsignal commented Jul 16, 2024

glsignal Jul 16, 2024

privatenumber Jul 16, 2024

privatenumber commented Jul 16, 2024

privatenumber commented Jul 16, 2024

privatenumber commented Jul 16, 2024

glsignal commented Jul 16, 2024

	if (resourcePath.match(/node_modules/) !== null) {
	if (resourcePath.includes('/node_modules/')) {

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

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

Conversation

glsignal commented Jul 13, 2024 • edited Loading

privatenumber commented Jul 13, 2024

glsignal commented Jul 13, 2024 • edited Loading

glsignal commented Jul 14, 2024 • edited Loading

privatenumber Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

glsignal Jul 15, 2024

Choose a reason for hiding this comment

privatenumber Jul 15, 2024

Choose a reason for hiding this comment

glsignal Jul 15, 2024

Choose a reason for hiding this comment

privatenumber commented Jul 15, 2024

glsignal commented Jul 15, 2024 • edited Loading

glsignal commented Jul 16, 2024

glsignal Jul 16, 2024

Choose a reason for hiding this comment

privatenumber Jul 16, 2024

Choose a reason for hiding this comment

privatenumber commented Jul 16, 2024

privatenumber commented Jul 16, 2024

privatenumber commented Jul 16, 2024

glsignal commented Jul 16, 2024

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

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

glsignal commented Jul 13, 2024 •

edited

Loading

glsignal commented Jul 13, 2024 •

edited

Loading

glsignal commented Jul 14, 2024 •

edited

Loading

privatenumber Jul 15, 2024 •

edited

Loading

glsignal commented Jul 15, 2024 •

edited

Loading