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(tsconfig): parse extended tsconfigs when transpiling script blocks #502

Merged
merged 2 commits into from Nov 3, 2022

Conversation

thebanjomatic
Copy link
Contributor

A change introduced in v28.1.0 in PR #471 unintentionally changed the behavior of the tsconfig parsing such that configs using "extends" were no longer being considered.

Fixes: #495

@thebanjomatic
Copy link
Contributor Author

thebanjomatic commented Oct 11, 2022

@lmiller1990 I wouldn't mind back-patching this to vue-jest v28.x also, but there isn't a branch to target for that. What is the process for backpatching? I support infrastructure that wraps around these tools and I'm currently having to workaround this problem in my project's v28 branch, so it would be nice to be able to remove the workaround in both places.

[Update] I'm just going to release jest 28 and jest 29 support for my package using the workaround. I will probably update the jest 29 release to not use the workaround if/when this PR gets released, but will probably just leave the jest 28 implementation alone.

@thebanjomatic thebanjomatic force-pushed the fix/tsconfig-extends branch 3 times, most recently from 6f7d17e to 61a57b3 Compare October 11, 2022 19:27
ensureRequire('typescript', ['typescript'])
const typescript = require('typescript')

const tsconfigPath = resolveTsConfigSync(process.cwd(), path || '')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: we are still using the tsconfig package to resolve the path to the tsconfig file in the same way that tsconfig.loadSync was previously doing.

Comment on lines +99 to +106
onUnRecoverableConfigFileDiagnostic: e => {
const errorMessage = typescript.formatDiagnostic(e, {
getCurrentDirectory: () => process.cwd(),
getNewLine: () => `\n`,
getCanonicalFileName: file => file.replace(/\\/g, '/')
})
warn(errorMessage)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will provide us with an error message if typescript is unable to read the tsconfig file or one of the extended files because they don't exist or don't parse etc.

Copy link
Member

Choose a reason for hiding this comment

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

This is nice, I like good error messages.

Comment on lines 81 to 83
if (path in tsConfigCache) {
return tsConfigCache[path]
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: Return early from cache if we have already encountered that path.

I don't know how expensive ts.getParsedCommandLineOfConfigFile is, or if typescript is doing any caching internally, but I typically only call this once per-project rather than per source file when I've used it in the past.

Since getTypescriptConfig is also called for each interpolated script block in the template as well as the <script> block of the SFC, it seemed like a good idea to just cache the parsed config result and reuse it.

Copy link
Member

Choose a reason for hiding this comment

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

Nice

Comment on lines +112 to +115
const transpileConfig = {
compilerOptions: {
...compilerOptions,
module: typescript.ModuleKind.CommonJS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: As in the old code, we override the module with commonjs. I changed this to use the enum instead of a string literal because it failed to compile in my typescript version of this code. The expected value of CompilerOptions['module'] is ModuleKind, but it does seem to accept the string literals at runtime.

@@ -1,4 +1,4 @@
const throwError = require('./utils').throwError
const throwError = require('./throw-error')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: In order to use ensure-require from within utils.js, I needed to break the circular dependency. This was done by moving throw-error out of utils and into its own module so that ensure-require doesn't depend on utils.

Comment on lines +17 to +18
// these conditions are not being met and happen to be the use-case which was triggering errors
// in my config setup.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my project, we were setting esModuleInterop in a shared base config, and Vue.extend(...) was throwing a TypeError when running the test because it "could not read property 'extend' from undefined".

@thebanjomatic
Copy link
Contributor Author

@lmiller1990 Have you had the chance to look at this PR?

@lmiller1990
Copy link
Member

@thebanjomatic I was offline for all of Oct. I'm back now, let me take a look.

Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

This mostly looks good to me. I left a few nits - I'll give you a moment to respond, then we can merge this up and ship as 29.x.

We can look to do a 28.x back merge... I guess we will just fork from the v28 tag (I think we have one?) and back merge.

e2e/2.x/basic/components/ExtendedTsConfig.vue Outdated Show resolved Hide resolved
Comment on lines 81 to 83
if (path in tsConfigCache) {
return tsConfigCache[path]
}
Copy link
Member

Choose a reason for hiding this comment

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

Nice

packages/vue2-jest/lib/utils.js Outdated Show resolved Hide resolved
Comment on lines +99 to +106
onUnRecoverableConfigFileDiagnostic: e => {
const errorMessage = typescript.formatDiagnostic(e, {
getCurrentDirectory: () => process.cwd(),
getNewLine: () => `\n`,
getCanonicalFileName: file => file.replace(/\\/g, '/')
})
warn(errorMessage)
}
Copy link
Member

Choose a reason for hiding this comment

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

This is nice, I like good error messages.

Adam Hines added 2 commits November 1, 2022 14:23
A change introduced in v28.1.0 in PR vuejs#471 unintentionally changed the behavior of the tsconfig parsing such that configs using "extends" were no longer being considered.

Fixes: vuejs#495
@thebanjomatic
Copy link
Contributor Author

Thanks @lmiller1990 I have updated this PR with your suggested changes. I'm no longer personally in need of the jest 28 release unless you'd just prefer to patch it because it was broken in v28.1.0

@lmiller1990 lmiller1990 merged commit bb516da into vuejs:master Nov 3, 2022
@lmiller1990
Copy link
Member

Done https://github.com/vuejs/vue-jest/releases/tag/v29.2.0

LMK if you have any issues.

@thebanjomatic thebanjomatic deleted the fix/tsconfig-extends branch November 3, 2022 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New tsconfig loading feature doesn't respect tsconfig extends
2 participants