microsoft / rushstack

Monorepo for tools developed by the Rush Stack community
https://rushstack.io/
Other
5.81k stars 592 forks source link

[heft] copy-static-assets gets confused about outdir if it's more than 2 levels deep in extends #2617

Open scamden opened 3 years ago

scamden commented 3 years ago

Summary

If my tsconfig.json extends one that extends another which has the outDir defined, copy static assets doesn't correctly determine the outDir

Repro steps

https://github.com/creditiq/simple-repros/tree/copy-static-assets-out-dir-bug

  1. git clone the above
  2. cd simple-repros
  3. rush update
  4. rush build

    Expected result: apps/main-app/dist/app contains style.scss

    Actual result:

root directory of monorepo contains dist/app/style.scss instead.

if you comment in the duplicate outDir in libraries/rig-rig-test/profiles/web-app/tsconfig-app.json the file shows up where it should

Question Answer
@rushstack/heft version? 0.28.3
Operating system? Mac
Would you consider contributing a PR? Yes
Node.js version (node -v)? 14.16.0
iclanton commented 3 years ago

The outDir path is relative to the folder of the config file in which it's specified. Additionally, when secondary extensions in config files are resolved, the path is realpath'd, so it the outDir ends up resolving to the root of the repo because that's four levels (../../../../dist) below the libraries/rig-rig-test/profiles/default/tsconfig-base.json folder's path.

We can probably add an option to heft-config-file to try to not realpath the extends field, but that won't work in all scenarios. Otherwise, we can special-case reading the tsconfig file and not use heft-config-file at all. @scamden, what do you think? Would you be interested in making a PR?

scamden commented 3 years ago

Ya so I followed heft's rig example using these relative paths.

At first it didn't work and went relative to the real path of the tsconfig.json (even for TS). It only started working when I used a ./node_modules/ instead of just node_modules in my extends, which leads me to think that ts is resolving to the real path in the second case but not in the first. I thought that was weird and makes me worry that I'm relying on a quirk of ts's implementation. Ultimately, this is only a problem because of symlinks though, otherwise the real path would be in node_modules in both cases. Anyway, all that as backstory and just something I was a little weirded out by.

What I don't understand is why copy-assets works for the second level config but not for the third. If I ran a real path on either one and then resolved the relative path, it would result in the root directory of mono repo. They are at the same directory level in the tree. So is it running real path in one case but not in the other?

If you can point me at the right spot to look in the code, I'd be more than happy to attempt a PR.

iclanton commented 3 years ago

The ./node_modules/ vs node_modules thing is interesting. At least for some versions of TS, the extends parameter has to be a relative path, so TS won't resolve a package name. I would think if you said node_modules/rig-package-name/.../tsconfig-base.json, it would try to find a dependency called "node_modules," and then just fail. The path used by copy-static-assets is computed by heft-config-file, and the outDir property used by the TS builder is computed by TypeScript itself, so there are definitely slight differences in the algorithms.

I'm honestly not sure why it works for second- but not third-level. We'd have to look through some of the edge cases of heft-config-file. I wrote that package, but I'm not sure offhand why it's doing that :P.

It'd be awesome if you wanted to take a stab at fixing this! This is the function that returns the outDir path: https://github.com/microsoft/rushstack/blob/master/apps/heft/src/plugins/CopyStaticAssetsPlugin.ts#L141-L152 and this is the thing that actually loads the tsconfig file: https://github.com/microsoft/rushstack/blob/master/apps/heft/src/plugins/CopyStaticAssetsPlugin.ts#L32-L62

scamden commented 3 years ago

I wrote that package, but I'm not sure offhand why it's doing that :P.

haha all too familiar :) cool i'll take a stab when i can. might be a bit since we have a workaround but it'll be a good chance to familiarize myself more with this stuff