-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Allow 'paths' without 'baseUrl' #40101
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
Allow 'paths' without 'baseUrl' #40101
Conversation
a1427a6
to
a5b1b5e
Compare
// the config file location, we'll need to know where that config file was. | ||
// Since 'paths' can be inherited from an extended config in another directory, | ||
// we wouldn't know which directory to use unless we store it here. | ||
ownConfig.options.pathsBasePath = basePath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this should be non enumerable and should be in tsconfig build info or not. You would also want test case with incremental set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a baseline that shows it in the .tsbuildinfo. It seems like it would be necessary if an incremental build doesn’t re-resolve everything in tsconfig, but I’m not sure how that works. Exactly what would you want to see in a test case?
tests/baselines/reference/pathMappingBasedModuleResolution1_amd.errors.txt
Outdated
Show resolved
Hide resolved
Not to pester you too much, but if possible I'd love to see a test for my case in #31869 (comment) (which you said should work), just so that doesn't end up regressing. 🙂 |
@jakebailey there are a couple other auto-import issues related to |
Sure thing, I wasn't sure if this small change managed to do it all. Thanks. |
4f77b4d
to
4a2f282
Compare
@andrewbranch I can't tell but it looks like this PR is waiting on an answer to @sheetalkamat's last question. Is that correct? |
I think I’m waiting on answers to my responses to Sheetal: The latter I didn’t exactly phrase as a question, I guess because I was hoping someone would have a better idea on what to do when we construct a program with |
trace(state.host, Diagnostics.paths_option_is_specified_looking_for_a_pattern_to_match_module_name_0, moduleName); | ||
} | ||
return tryLoadModuleUsingPaths(extensions, moduleName, baseUrl, paths, loader, /*onlyRecordFailures*/ false, state); | ||
const baseDirectory = baseUrl ?? Debug.checkDefined(pathsBasePath || state.host.getCurrentDirectory?.(), "Encountered 'paths' without a 'baseUrl', config file, or host 'getCurrentDirectory'."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks sketchy, but I manually verified that the assertion should hold in all cases we currently have; it’s just hard to encode it into the types. The only times host.getCurrentDirectory
is undefined
are
- The command line parser and typings installer each call the module name resolver in an ad-hoc way under certain circumstances, but they pass a hard-coded
compilerOptions
that lackpaths
, so this code path is never taken. Project
callsgetAutomaticTypeDirectives
with aDirectoryStructureHost
, but that function never useshost.getCurrentDirectory()
.
Changing ModuleResolutionHost['getCurrentDirectory']
to be non-optional makes these call sites a bit of a nuisance.
361eb08
to
f3f82e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm not quite sure what the incremental test cases should look like (maybe a test where you add/remove the basePath
and/or paths
options?), but this looks OK to me. I don't know if you wanna do it in this PR, or as a followup, but this probably wants for us to change logic in src/compiler/moduleSpecifiers.ts
, too - that's what controls how we serialize module names for declaration emit, which should be affected by this new compiler option functionality, I'd think.
I’m having trouble observing this, because when you use I discovered an unrelated bug while trying to come up with a test case where the module specifiers generated for the module declaration blocks in an I do think the code in |
tsconfig behaves differently with `tsc` vs. Next.js. When `baseurl` is omitted, `paths` should be resolved against the tsconfig location. Consequentially, in this case the paths must start with `./`. This PR aligns Next.js behavior with `tsc` around `paths` without `baseurl`. Related: microsoft/TypeScript#40101
Allows the
paths
compiler option to be used withoutbaseUrl
by resolving paths relative to the config file location.baseUrl
were set, these so-called “non-relative” paths would still be resolved relative tobaseUrl
, so there’s never a reason to write a non-relative path in the first place. Explicitly disallowing it leaves room for us to add new, useful behavior later, if needed.)paths
is inherited from an extended config file in another directory, the relative paths therein are resolved relative to the config file where they were written, not relative to the extending config file. (I don’t think this even needs to be said; the result is that relative paths look correct for where they’re written.)Fixes #31869