Auto import of monorepo package not using node_modules specifier
andrewbranch opened this issue · 14 comments
@andrewbranch
Sure, here's a repo that has the issue.
This is based on a simplification of our own yarn monorepo. We've got a common project that contains stuff that's shared between frontend and backend (the backend project is omitted), and a web project that references it via a @common alias. Each project has its own tsconfig.json. I believe this is pretty similar to the example repo posted above. To see the problem, hop into web/src/Helper.ts and try to use a "Quick Fix" to import square from the common repo.
This example repo doesn't include any of this extra stuff because it suffices to show the issue, but for context, in our real repo web is a CRA-based Webpack app. We run it via yarn tsc --build --watch & react-app-rewired start (we run tsc separate because CRA's setup doesn't understand building projects). We want to import the compiled code in common/dist via an alias (e.g. import { square } from '@common/MyModule').
Our biggest issue is the one mentioned in the comment above - VSCode auto-import suggests relative paths to the original .ts file no matter what. There's a few issues here:
- it leads CRA to barf because of the attempt to import a file outside of the project (this works fine with aliases, with some monkey-grease)
- we want to be importing the compiled
.jsfile, not the original.tsfile - Depending on VSCode settings, it won't even import the correct file.
What I mean by that last point is, if I try it with the "Import Module Specifier" preference set to "shortest", it works (although with a relative path, which is still undesirable):
But if I set "Import Module Specifier" to "non-relative", it curiously gives me:
Which is simply broken - an immediate compilation error.
It's totally possible - and would be great if! - this is just a configuration issue on our part and there's a different/better way to do it.
Thanks for your help!
Originally posted by @adamsmasher in #39778 (comment)
Just checking in to see if there's any updates here 🙂
Also reaching out because I'm wondering/hoping if anyone familiar with setting up TypeScript monorepos/project references could have a look at that sample repo and let us know if we're doing something fundamentally wrong in our setup. We're hitting lots of rough edges in our tooling up-and-down the stack since trying to move to project references, mostly by cobbling together info from various blog posts; my hunch is that this is on our end - surely lots of folks have a setup like this and it works better for them - so we'd be really grateful if someone could help point us on the right track. Thanks!
Sorry, this slipped 4.9. I just took a quick look, and I don’t think it looks like you’re doing anything really wrong; I just see a couple of our own bugs that you pointed out. I did notice that the incorrect relative path bug goes away if you remove baseUrl from your settings. Long ago, baseUrl had to be set in order to use paths, but that hasn’t been true for some time. You shouldn’t set baseUrl unless you intend to be able to write imports like import {} from "src/Helper" (notice the lack of relative path there). The other minor oddity is the lack of a rootDir in your config files, which is why you get an output structure like dist/src/Helper.js instead of just dist/Helper.js, which you can achieve by setting "rootDir": "src". But in your case this may just be an aesthetic change if your bundler is already happy.
So there are two auto-import bugs here:
- The relative path offered when
baseUrlis set and the preference isnon-relativeis just wrong - Auto-imports fails to discover the
pathsalias
@andrewbranch one issue we are having... when auto-imports pick a correct relative path, but NOT the module path we want
e.g.
import { StockItem } from '../../../common/src/store/StoreTypes';
vs
import { StockItem } from '@common/store/StoreTypes';
they both compile, but if these both get used, it's very dangerous as they are considered (rightfully) by TypeScript to be different modules. If it's just types and functions it's fine. If there is anything stateful in the module, then it breaks since now there are 2 different instances.
as such, when the auto-import picks the relative path instead of the module, it causes bad problems. As such, we actually sort of WANT the auto-import to be broken if it's not the module one, so at least it's noticed to be wrong sooner.
That’s what I labeled as the second bug above. But TypeScript should recognize those as the same module, actually. Assuming the path mapping points to the output file, there is code that recognizes the mapping between referenced project outputs and inputs. From TypeScript’s perspective, you should be able to point to either the outputs or the inputs or both, and by default, tsc’s source of truth will be the output .d.ts file, while the language service’s source of truth will be the input .ts file. If that’s not happening, something else is going on that I didn’t detect in the example project from @adamsmasher.
@andrewbranch in our case, it definitely does not recognize them as the same module.
I'll spent some time this weekend experimenting with those suggestions and figuring out why the mapping you're talking about isn't working for us. Thanks for all your help @andrewbranch!
@bradedelman how are you determining that?
Also, you can try the fix at #51492 in the meantime. @typescript-bot will comment with an npm-installable tarball shortly.
Hm, #51492 isn’t quite right yet. I think it fixes your case but it lights up some other auto-imports that I think are undesirable. Let me ask this: why do you want to point your paths alias to your output folder?
we want to be importing the compiled
.jsfile, not the original.tsfile
paths is a TypeScript setting and TypeScript will work better if you point it to the original .ts file in this case. Are you doing this for the benefit of some other tool in your build?
Pushed an update, and it’s very hard to read intent from paths, but at least tests pass now. A new npm build is on the way.
Thanks again, this is great. Haven't tried it yet but I will ASAP.
As for why we're pointing paths to the output folder, it's based on the advice from this blog post:
Can you import the TypeScript Source instead of the JavaScript?
You can import the TypeScript source from your projects, but you probably should not. If you do set up your project to import the TypeScript, webpack will bundle your project just fine, but then you are not using project references. You have succeeded in organising your codebase but you are not getting the advantage of reducing build time by using the compiled files in
lib. In fact, you are slowing down your build by requiringtscor ts-loader to build the reference and then not using it.
It does seem to be the source of at least a few of our problems. Is this nonsense?
No, it’s not wrong, but it’s also not the only way to configure Webpack. You could forego tsconfig-paths-webpack-plugin and define aliases that Webpack is happy with in Webpack’s config and aliases that TypeScript is happy with in TypeScript’s config, at the cost of maintaining two separate configs. I understand why that’s unappealing, but it’s an option if things are giving you trouble. I think #51492 should let you keep your configs the way they are and fix auto-imports. That said, having been involved with trying to layer project references on top of ts-loader, it’s extremely complex and I would encourage you to test the author’s claims about performance gains for yourself, since every project is different, and the article is a couple years old.
@bradedelman I think some of the confusion here is that we're talking to the TypeScript team - the folks responsible for transpilation and language integration in VSCode - but the behaviour you're describing is a runtime behaviour that's under the purview of ts-node and/or webpack/babel-loader. From TypeScript's perspective, this mapping is working fine - that's how we've gotten debugging in VSCode, code search, etc. working; at runtime ts-node and (our very old version of) webpack aren't associating the compiled output and the original source files - I'm not sure if that's due to bugs on their end or misconfiguration on our part, but either way it might be out of scope here.
@andrewbranch the fix in #51492 seems to work great! Do you figure that build is stable enough that we could use it while we wait for the change to hit official release? If not, how far off do you think we'll be waiting? In the mean time, we'll try to fix up the baseUrl and rootDir settings here, and experiment with importing the .ts files instead of the transpiled .js files like you suggested to see how much it hurts performance and/or improves our tooling situation. Once again, super super grateful for all of your help with this.
but the behaviour you're describing is a runtime behaviour that's under the purview of ts-node and/or webpack/babel-loader. From TypeScript's perspective, this mapping is working fine - that's how we've gotten debugging in VSCode, code search, etc. working; at runtime ts-node and (our very old version of) webpack aren't associating the compiled output and the original source files - I'm not sure if that's due to bugs on their end or misconfiguration on our part, but either way it might be out of scope here.
Exactly right 👍
Do you figure that build is stable enough that we could use it while we wait for the change to hit official release?
So, once #51492 is merged, which should happen soon, it will be included in nightly typescript@next builds. Otherwise, you’ll be waiting for a while for 5.0, as we’re at the very beginning of the release cycle. I always love for folks to use the nightlies. I would encourage you to get onto those after the PR is merged. If something is broken, let us know, and that will make our 5.0 release better.