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

feat: respect tsconfig options that affects compilation results #4279

Merged
merged 6 commits into from
Jul 28, 2021

Conversation

sodatea
Copy link
Member

@sodatea sodatea commented Jul 16, 2021

Description

Closes #3227

Additional context


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.

@sodatea sodatea changed the title fix: respect tsconfig options that might change the compilation results feat: respect tsconfig options that might change the compilation results Jul 16, 2021
@Shinigami92 Shinigami92 added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jul 16, 2021
Shinigami92
Shinigami92 previously approved these changes Jul 17, 2021
@Shinigami92
Copy link
Member

Title says feat: ... but it's marked as fix? What is it? Maybe rename the title or use p2 🤔

@patak-dev patak-dev added this to the 2.5 milestone Jul 17, 2021
@sodatea
Copy link
Member Author

sodatea commented Jul 17, 2021

I changed the title to feat: after this commit 9d1c05f (#4279), as it looks more like a feature.

patak-dev
patak-dev previously approved these changes Jul 28, 2021
@patak-dev
Copy link
Member

@sodatea do you think we could merge this one without Evan's input? It would be good be able to include it in the 2.5 beta

@sodatea
Copy link
Member Author

sodatea commented Jul 28, 2021

I think so. This is the standard TypeScript behavior so we'll have to support it anyway.

The only problem here is that whether it will break users' projects. So including it in the beta first is a good idea.

@sodatea
Copy link
Member Author

sodatea commented Jul 28, 2021

Wait a moment though. I found a typo in my code 😂

@sodatea sodatea dismissed stale reviews from patak-dev and Shinigami92 via 8741ec8 July 28, 2021 08:15
@patak-dev patak-dev changed the title feat: respect tsconfig options that might change the compilation results feat: respect tsconfig options that affects compilation results Jul 28, 2021
@patak-dev patak-dev merged commit 2986f6e into vitejs:main Jul 28, 2021
return cached
}

let configPath = await findTSConfig(directory)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check the initial configPath whether is include in 'process.cwd' ?

Copy link
Member

Choose a reason for hiding this comment

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

Would you open an issue about this so it is easier to track its resolution?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, yes. I'm just not sure about the problem I met is worth solving.
I create a typescript project that doesn't have tsconfig.json in workspace. And findTSConfig is trying to recursively find tsconfig.json and it finds a tsconfig.json in some parents folder. That tsconfig.json#target is es5, so when I run yarn vite, I get an error for esbuild doesn't support es5, which I knew but doesn't know what causes the error until I look into the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's from esbuild, not this PR.

Because in this PR I only add check for 4 fields: jsxFactory, jsxFragmentFactory, useDefineForClassFields, and importsNotUsedAsValues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tsconfig useDefineForClassFields setting not respected
4 participants