-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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(deps): downgrade tsconfck #15899
fix(deps): downgrade tsconfck #15899
Conversation
Run & review this pull request in StackBlitz Codeflow. |
the newer version fixes a bug that could lead to silent use of incompletely extended tsconfig so i don't think this is the right way. |
see this comment how to restructure tsconfig with references to avoid it until fixed aleclarson/vite-tsconfig-paths#132 (comment) |
The main problem is in the qwik-nx package, that creates a few tsconfigs per qwik project. Of course I could fix it for my own project, but I don't think it is the right way to propagate workarounds to the consumers of the package. There might be bugs that are fixed by the Right now I'm limiting the vite version there to the |
regardless of the outcome here qwik-nx should not create these loopy references+extends combination in my opinion. typescript itself does not use this pattern. users who are affected by the new issue it created are few in comparison to all users affected by the fix. and they have 2 workarounds. downgrade vite or improve their tsconfig setup |
@dominikg AFAIK this works as expected. nx projects are using TS solution config. learn more here https://github.com/microsoft/fluentui/blob/master/rfcs/shared/build-system/02-solution-style-ts-configs.md#detailed-design-or-proposal |
i feel like we are talking past each other. tsconfck supports solution style tsconfig files. https://github.com/dominikg/tsconfck/tree/main/packages/tsconfck/tests/fixtures/parse/solution the bug only affects solutions that have a loop where a referenced tsconfig extends the same file that references it, not all solutions. if you have solutions without a loop that get stuck please file a new bug with reproduction on tsconfck. |
@dominikg I understand this
Along with that you're saying
My point is that the amount of users who rely on this approach is not small in any sense, this is a pattern that is widely used by nx itself. I feel like this bug hasn't fully reached everyone as vite@5.1 is very recent and that's why it does not get broader attention. Just hear me out: if a bugfix creates a regression that is barely fixable on the consumer's side, it's not a right bugfix. TS itself works just fine with extends/references loops, so this is something that From my understanding, in Nx's setup, the combination of references and extends serves two purposes:
So I wouldn't say it should be "fixed" as it is a functional setup that does the job |
I agree with @dmitry-stepanenko The Nx has the global The These configurations If the references to The above suggested solution aleclarson/vite-tsconfig-paths#132 (comment), is to move the |
this new bug is very visible and easy to avoid. The bug that was fixed is not. It silently used an incompletely extended tsconfig, possibly leading to build output you don't expect. It also affects all users using extends, not only those that use a combination of extends and references that create a loop. I'm done arguing here. Maybe you could spend time investigating how to contribute a fix to tsconfck instead of just reverting to a known broken state just because it might not break yourself? If this can't be solved in the short term, another possibility is to reduce the amount of caching done inside tsconfck to sidestep the freeze. I'd take degrading performance over reintroducing a bug. |
I agree that would be the best solution for now. I saw an opened PR in the tsconfck repo which does exactly that, can I assume this is something that you're going to release in the nearest future? |
Closing as #16124 is merged (released in 5.1.6). |
Description
tsconfck has been updated as part of #15803. New version of the package causes qwik build to fail when tsconfig is used with "references". It might affect other frameworks or use cases, but I haven't tested those.
Looks like this is a known issue on the package's side dominikg/tsconfck#154, however it remains not fixed as of now. The suggestion is to keep the dependency on the lower version that does not cause any issues
Please see reproduction details on the related issue #15870
Additional context
closes #15870
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).