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

fix: support dangling comma and throw on circular dependency in tsconfig #4590

Merged
merged 5 commits into from Aug 14, 2021

Conversation

sodatea
Copy link
Member

@sodatea sodatea commented Aug 12, 2021

Description

By implementing tsconfig resolve/parse/load logic in vite.
Much of the code is from the tsconfig package and thanks to @dominikg

Fixes #4481
Fixes #4512

Additional context

strip-bom v4 is used instead of v5, because v5 is a native ES module, causing jest tests to fail.

No tests are added for the circular dependency issue because I don't yet know how to test it.

If we need better error messages, json5 can be used.
I've considered using jsonc-parser, but it outputs error messages for trailing commas despite successfully parsing it.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

By implementing tsconfig resolve/parse/load logic in vite.
Much of the code is from the `tsconfig` package and thanks to @dominikg

Fixes vitejs#4481
Fixes vitejs#4512
@sodatea sodatea requested a review from antfu August 12, 2021 11:58
@sodatea
Copy link
Member Author

sodatea commented Aug 12, 2021

😂 I've added the comma! It was formatted by prettier…
Should add prettier-ignore to the file

@dominikg
Copy link
Contributor

i've played with the idea of spinning tsconfig-parse as a new package - a more modern version of tsconfig package if you will.

see also evanw/esbuild#1515 (comment)

let me know if you'd be fine with me reusing the resolution code and if you'd be willing to swap it out in vite then.

@sodatea
Copy link
Member Author

sodatea commented Aug 12, 2021

Yeah, that would be cool!

I worked on this PR mainly to unblock the release of v2.5.
A dedicated package would be better.

@dominikg
Copy link
Contributor

Regarding error reporting, JSON.parse error position should match the position in invalid tsconfig.json content so this should be enough here. Semantic errors would need either tsconfig schema validation or again using typescript api. I'll think about adding that to the package at some point then ;)

@Shinigami92 Shinigami92 added the p2-edge-case Bug, but has workaround or limited in scope (priority) label Aug 12, 2021
packages/vite/src/node/plugins/esbuild.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/esbuild.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/esbuild.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/esbuild.ts Show resolved Hide resolved
@antfu antfu merged commit f318416 into vitejs:main Aug 14, 2021
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
…fig (vitejs#4590)

Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-edge-case Bug, but has workaround or limited in scope (priority)
Projects
None yet
4 participants