feat(dev-server-esbuild): allow extended tsconfig #2161

venikx · 2023-03-15T17:59:06Z

What I did

Added a test case when referencing a tsconfig.json in the esbuildPlugin that uses extends to extend from a base tsconfig.json
Added a possible implementation how it could work using --showConfig when running tsc. See https://www.typescriptlang.org/docs/handbook/compiler-options.html
Change the target of the fixture, since es2022 was reported as invalid by tsc

I'm not sure about the actual implementation, but it covers my test case, without breaking the other tests of dev-server-esbuild (at least on Mac) 🤷🏽

changeset-bot · 2023-03-15T17:59:09Z

🦋 Changeset detected

Latest commit: 99ae905

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@web/dev-server-esbuild	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

venikx · 2023-03-15T18:00:31Z

changeset-bot message...

So if I understand this correctly, I can trigger an automatic release of the package, by applying a change set?

Westbrook · 2023-03-15T19:12:49Z

Apply the changeset to clarify that there are changes requiring a release.

Full the changeset with a lingual representation of the changes so that they are included in the CHANGELOG.md.

venikx · 2023-03-17T22:05:08Z

Apply the changeset to clarify that there are changes requiring a release.

Full the changeset with a lingual representation of the changes so that they are included in the CHANGELOG.md.

Let me know if the formatting of the changeset should be adjusted!

koddsson · 2023-03-31T07:26:16Z

packages/dev-server-esbuild/src/EsbuildPlugin.ts

@@ -61,7 +62,15 @@ export class EsbuildPlugin implements Plugin {
    this.config = config;
    this.logger = logger;
    if (this.esbuildConfig.tsconfig) {
-      this.tsconfigRaw = await promisify(fs.readFile)(this.esbuildConfig.tsconfig, 'utf8');
+      try {
+        const { stderr, stdout } = await promisify(child_process.exec)(


Does reading the file not work? We have to shell out to tsc?

Reading 1 tsconfig.json file works, but only if you only have 1 tsconfig.json, without extending from another another tsconfig.json file.

Technically you could try parsing the file that you initially read, and then recursively call the same function over and over again until all the extends's are resolved. This is exactly what tsc --show-config does, so I chose to lean on that.

Oh, interesting! I'm wondering if we should really be using the .tsconfig setter instead of .tsconfigRaw. I'm not sure, but from reading the docs, it seems like that setting will work with extending configs?

If not, I think this is good!

What do you mean with the .tsconfig setter? And are you referring to the dev-server-esbuild setting, or esbuild itself?

I think I tested this already, but I can try it again. If you revert my change from the code, but keep the test you’ll get a failing test.

From dev-server-esbuild side we can’t set a .tsconfigRaw. We pass in the .tsconfig, convert it and then pass it as tsconfigRaw to esbuild.

Logically, I’d think .tsconfigRaw would use the tsconfig as you passed it in, without expanding it further automagically.

@koddsson I just tested this again, and with the test case I added the test fails, without the change I've proposed here.

venikx · 2023-04-17T20:46:09Z

I rebased the PR to be up to date with latest changes.

koddsson

This is great! I'm slightly worried about performance here since we'll be shelling out to tsc but it's probably fine.

We should probably have some performance testing in the future to help us get insights into how changes affect performance.

koddsson · 2023-04-18T06:22:39Z

We've got #2180 queued up to merge to master soon. I want to make sure we land that first so that we don't delay that release any more. We'll merge this once that PR has landed.

venikx · 2023-04-18T10:05:44Z

No worries, I've come across another way to possibly solve it while going through the dependabot PR's at work. Would something like this be preferred over shelling out tsc? qmhc/vite-plugin-dts#202

You are still using the typescript ecosystem, but have to do a bit more manual recursion in favour of not having to run a command.

EDIT: Also sorry for the re-review, I was worried you might not have seen this message, before potentially merging this PR.

koddsson · 2023-04-18T17:13:51Z

No worries, I've come across another way to possibly solve it while going through the dependabot PR's at work. Would something like this be preferred over shelling out tsc? qmhc/vite-plugin-dts#202

Oh, yeah! I would love to do this instead. Looks like we need to upgrade to TypeScript 5 first, which I think is also a positive, so I'd prefer that.

What do you think?

venikx · 2023-04-20T10:22:07Z

No worries, I've come across another way to possibly solve it while going through the dependabot PR's at work. Would something like this be preferred over shelling out tsc? qmhc/vite-plugin-dts#202

Oh, yeah! I would love to do this instead. Looks like we need to upgrade to TypeScript 5 first, which I think is also a positive, so I'd prefer that.

What do you think?

Yeah, I think it might be better, but it does indeed seem to require typescript 5. Would that then be considered a breaking change for dev-server-esbuild?

koddsson · 2023-04-22T19:33:05Z

Would that then be considered a breaking change for dev-server-esbuild?

Probably! But that's no worries, we can just make a breaking change.

koddsson · 2023-04-25T07:32:03Z

We should be on TypeScript 5 now if you pull master 👯

venikx · 2023-04-25T08:24:30Z

We should be on TypeScript 5 now if you pull master 👯

I'll modify my PR somewhere this weekend, I have busy schedule for this week unfortunately.

koddsson · 2023-04-26T07:11:47Z

We should be on TypeScript 5 now if you pull master 👯

I'll modify my PR somewhere this weekend, I have busy schedule for this week unfortunately.

No worries! Let me know if we can help :)

venikx force-pushed the feat/extends-tsconfig branch from 7cfa66c to 4fc32b2 Compare March 16, 2023 08:21

venikx mentioned this pull request Mar 16, 2023

[@web/dev-server-esbuild] does not read tsconfig.json #1921

Open

koddsson reviewed Mar 31, 2023

View reviewed changes

venikx added 3 commits April 17, 2023 23:28

test(dev-server-esbuild): add extends tsconfig

a32b1dd

feat(dev-server-esbuild): allow extended tsconfig

e6761ea

chore(changeset): generate changeset

99ae905

venikx force-pushed the feat/extends-tsconfig branch from ff9f749 to 99ae905 Compare April 17, 2023 20:44

venikx requested a review from koddsson April 17, 2023 20:46

koddsson approved these changes Apr 18, 2023

View reviewed changes

venikx requested a review from koddsson April 18, 2023 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dev-server-esbuild): allow extended tsconfig #2161

feat(dev-server-esbuild): allow extended tsconfig #2161

venikx commented Mar 15, 2023 •

edited

Loading

changeset-bot bot commented Mar 15, 2023 •

edited

Loading

venikx commented Mar 15, 2023 •

edited

Loading

Westbrook commented Mar 15, 2023

venikx commented Mar 17, 2023

koddsson Mar 31, 2023

venikx Mar 31, 2023 •

edited

Loading

koddsson Apr 3, 2023

venikx Apr 4, 2023 •

edited

Loading

venikx Apr 17, 2023

venikx commented Apr 17, 2023

koddsson left a comment

koddsson commented Apr 18, 2023

venikx commented Apr 18, 2023 •

edited

Loading

koddsson commented Apr 18, 2023

venikx commented Apr 20, 2023

koddsson commented Apr 22, 2023

koddsson commented Apr 25, 2023

venikx commented Apr 25, 2023

koddsson commented Apr 26, 2023

feat(dev-server-esbuild): allow extended tsconfig #2161

Are you sure you want to change the base?

feat(dev-server-esbuild): allow extended tsconfig #2161

Conversation

venikx commented Mar 15, 2023 • edited Loading

What I did

changeset-bot bot commented Mar 15, 2023 • edited Loading

🦋 Changeset detected

venikx commented Mar 15, 2023 • edited Loading

Westbrook commented Mar 15, 2023

venikx commented Mar 17, 2023

koddsson Mar 31, 2023

Choose a reason for hiding this comment

venikx Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

koddsson Apr 3, 2023

Choose a reason for hiding this comment

venikx Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

venikx Apr 17, 2023

Choose a reason for hiding this comment

venikx commented Apr 17, 2023

koddsson left a comment

Choose a reason for hiding this comment

koddsson commented Apr 18, 2023

venikx commented Apr 18, 2023 • edited Loading

koddsson commented Apr 18, 2023

venikx commented Apr 20, 2023

koddsson commented Apr 22, 2023

koddsson commented Apr 25, 2023

venikx commented Apr 25, 2023

koddsson commented Apr 26, 2023

venikx commented Mar 15, 2023 •

edited

Loading

changeset-bot bot commented Mar 15, 2023 •

edited

Loading

venikx commented Mar 15, 2023 •

edited

Loading

venikx Mar 31, 2023 •

edited

Loading

venikx Apr 4, 2023 •

edited

Loading

venikx commented Apr 18, 2023 •

edited

Loading