Set module resolution based on baseUrl in jsconfig/tsconfig.json

[robertvansteen]

Based on this thread on Twitter here is a PR to kickstart development/discussion on supporting absolute paths in Typescript via NODE_PATH so they work consistently between JS & TS.

Updated: Based on the discussion here: #5645 (comment) This attempts to unify the configuration by reading baseUrl from jsconfig.json / tsconfig.json.

🚧 This PR is still in progress

To do:

• Add tests
• Make sure paths are valid TypeScript
• Add deprecation message to react-scripts start
• Add documentation

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[gaearon]

@ is likely a larger chunk of work so it's better to do separately.

We'll need to test Windows.

---

While we're at it, I also have an alternative proposal: #5645 (comment). What do you think about it?

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[robertvansteen]

I'm having some difficulties with the tests both locally and on the CI. Anyone that has some experience with the test suite in CRA that has some time to help me out?

[IIFE]

@rovansteen I've just added few comments to the changes (first time reviewing open source code :) ).

Hope they make sense.

[robertvansteen]

@IIFE thanks for your feedback! 😄

[mrmckeb]

@rovansteen, great work - thanks!

If we're deprecating NODE_PATH, perhaps we should look at adding some warning here - or migration steps?

[robertvansteen]

@mrmckeb fair enough. Got any ideas about warnings or migration steps?

[mrmckeb]

@rovansteen I'd just check if NODE_PATH was passed in, and if it was, add a warning to the console and mention that it's been deprecated in favour of using either a jsconfig or tsconfig file to achieve the same thing - and it would be great to link through to any related docs.

The concern here is that some people (including me) already rely on NODE_PATH, and might not read the release notes carefully enough when updating to a new react-scripts version.

[robertvansteen]

So as @mrmckeb correctly pointed out the current implementation in this PR is not actually valid TypeScript and fails if you run tsc. A path (alias) should actually look like this: "@*": ["src/*"] rather than this: "@": ["src"].

I'm working on an implementation that supports this but @Timer is there any way we can run tsc on the test/fixtures/typescript folder, to make sure that even though it builds properly it's also completely valid TypeScript? The whole test setup seems a bit daunting to me so not sure where I should put that.

[mrmckeb]

Perhaps add a test with * along with @* for compatibility with the previous NODE_PATH approach.

[robertvansteen]

For people that had their NODE_PATH set to . and were importing like this: src/components/button?

[mrmckeb]

So, I think it should be the below as a default, as that matches the NODE_PATH behaviour. Assuming the user set NODE_PATH to src would make this a good default:

  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "*": ["src/*"],
    }
  },

Then, a user could do the following, if they chose.

  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "@*": ["src/*"],
    }
  },

[robertvansteen]

Hmm, I'm not sure if we should provide a default as the NODE_PATH is not set to . by default which would potentially confuse users thinking that's default node behavior.

[mrmckeb]

I understand, by default there's no NODE_PATH. However, I think most people that use it would use it with src.

As per most implementations that I've seen (based on Microsoft examples) you can leave "baseUrl" as ".", and the aliasing itself can happen in paths.

[robertvansteen]

Yes, that makes sense and that's something we should definitely recommend in the documentation. Especially if you are migrating from NODE_PATH=src

[robertvansteen]

@Timer @mrmckeb @ianschmitz given the vast amount of different tsconfig/jsconfig.json inputs we'd have to parse is there any chance we could add some unit tests for this? Right now CRA only contains integration/e2e tests but they are not practical to test lots of different configurations in this case.

I'm not really familiar with the whole testing setup in bash, so not sure where to add any unit tests. If you could give me a direction that would be really helpful!

[mrmckeb]

Hi @rovansteen, I think @Timer would be better placed to give direction on this - as he's been with the project a lot longer than I. However, I'll try to answer as best I can :)

I think you would probably want to add a fixture here:
https://github.com/facebook/create-react-app/tree/master/test/fixtures

Essentially, you'd be testing that the app picks up the configs as expected.

[robertvansteen]

@mrmckeb I have added that, however, there are so many possible configurations we should support it's not feasible to run an integration test for every possible configuration. I'd rather have an integration test to check if the config is picked up as expected (I already added these) and then a bunch of unit tests that just test if the values of the configuration are parsed correctly.

[eps1lon]

As far as I can tell this would change the behavior of paths explained in microsoft/TypeScript#10866. In depth comment: #5645 (comment)

[robertvansteen]

@eps1lon good point, thanks for bringing that up.

@gaearon you've been an advocate for adding some sort of alias for absolute imports. Do you still want to add that to CRA even if TypeScript's support to this is limited to just compilation?

[robertvansteen]

Since this PR became a bit too big and ambitious by trying to support both baseUrl and aliases I've created a new PR just for supporting baseUrl (#6656). Giving us more room to figure out how to add support for aliases in a stable way.