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

Use "paths" in tsconfig.json and jsconfig.json #10014

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

sheepsteak
Copy link
Contributor

@sheepsteak sheepsteak commented Nov 4, 2020

Adds support for compilerOptions.paths in both tsconfig.json and jsconfig.json so that path mapping can be used:

"compilerOptions": {
  "baseUrl": "src",
  "paths": {
    "base/*": ["./components/base/*"],
    "pages/*": ["./components/pages/*"],
    "actions/*": ["./state/actions/*"]
  }
}
  • Parse paths and add to Jest aliases
  • Parse paths and add to Webpack aliases
  • Show error when paths is used without baseUrl for TypeScript before 4.1
  • Show error when paths is used improperly (has to be like example above with only one possible location; no extra fallbacks as Webpack 4 can't handle it)
  • Add docs

I've tried different combinations of paths in both JavaScript and TypeScript projects but no doubt there are still some bugs. I'd just like to get some early feedback and find out what the appropriate level of testing is.

Closes #5645
Closes #9406
Closes #9999

`${chalk.cyan('paths')} requires ${chalk.cyan('baseUrl')} to be set`
);
}
}
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 is to allow for the fact that TypeScript 4.1 does not need baseUrl to use paths.

@@ -262,6 +263,14 @@ function verifyTypeScriptSetup() {
);
}

Copy link

Choose a reason for hiding this comment

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

Should also probably warn if a user for whatever reason decides to use more than one path (eg: ["components/*", "layouts/*"]). What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've just done this. I tried implementing multiple path support first but it seems like Webpack 4 does not support this. Webpack 5 does though so when CRA moves to that we can add this feature.

I've now created an errors array that I push strings into. I can't use the current messages array as they are just outputted as information that your tsconfig.json has been updated. If the baseUrl or paths are set up incorrectly I need to process.exit(1) as we shouldn't go any further.

@@ -165,6 +164,7 @@ function verifyTypeScriptSetup() {
getNewLine: () => os.EOL,
};

const errors = [];
Copy link
Contributor Author

@sheepsteak sheepsteak Nov 18, 2020

Choose a reason for hiding this comment

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

All the changes I've made in this file should equally be applied to any jsconfig.json that is being used. However, there is no file that does at the moment. Should we have a verifyJavaScriptSetup.js? 👀

@pd8
Copy link

pd8 commented Nov 26, 2020

This is a really useful change that would remove the need to use CRACO or React App Rewired in our application (and assumedly many apps). CRA-TS used to support this but that project is now deprecated so it would be great to get this into the main project.

Is there any movement on this functionality CRA team?

@merceyz
Copy link
Contributor

merceyz commented Dec 8, 2020

Just to note: if you're using Yarn 2 you can already do this using the link: protocol
https://yarnpkg.com/features/protocols
https://yarnpkg.com/features/protocols#why-is-the-link-protocol-recommended-over-aliases-for-path-mapping

@pnewhook
Copy link

pnewhook commented Feb 7, 2021

What's outstanding for this to be merged? I'd like to share a project of interfaces between an express app and a CRA frontend stored in the same directory.

@danielo515
Copy link

Hey, can someone move this forward?
The more you leave it the harder it gets to merge

@sheepsteak
Copy link
Contributor Author

What's outstanding for this to be merged?

The checkboxes in the description are up to date. More importantly, I still need some guidance from the CRA team around whether we need to start verifying the jsconfig.json file like we do with the tsconfig.json and also on the overall approach; it just needs a review basically to see if they'll accept it and then I get some tests done.

The related issue for this (#5645) has been placed into the 4.x milestone and that seems really far away at the moment due to the amount of PRs in the other milestones. I'm not sure if this is a priority for the team at the moment.

@keremciu
Copy link

keremciu commented Apr 7, 2021

this would be a great addition

@jayarjo
Copy link

jayarjo commented May 5, 2021

We also wait for this to progress.

@iansu iansu self-assigned this May 12, 2021
@EricRabil
Copy link

EricRabil commented May 16, 2021

I was very disheartened to discover how long this has taken to be fixed. Regardless, here's my patch. It's ugly, it requires craco, it may break with a later CRA update, and it assumes you are using aliases how I am (my usage is shown).

It does the following:

  • Grabs an original copy of tsconfig
  • Wraps verifyTypeScriptSetup using require.cache
  • After verifyTypeScriptSetup runs it restores the paths you configured
  • Injects your tsconfig paths into webpack so they are properly resolved

craco.config.js

const verifyTypeScriptSetup = require("react-scripts/scripts/utils/verifyTypeScriptSetup");
const fs = require("fs");
const os = require("os");
const path = require("path");

const mapObject = (obj, mapper) => Object.fromEntries(Object.entries(obj).map(mapper));

// original copy of tsconfig
const originalTSConfig = require("./tsconfig.json");

// generates a mapping of aliases that webpack can take
const webpackCompatibleAliases = mapObject(originalTSConfig.compilerOptions.paths, ([ k, [ v ] ]) => [
    k.slice(0, -2), path.resolve(__dirname, originalTSConfig.compilerOptions.baseUrl, v.slice(0, -1))
]);

// wrap the utility function that is ruining the party!! >:(
require.cache[require.resolve("react-scripts/scripts/utils/verifyTypeScriptSetup")].exports = function() {
    // let cra think they ruined the party
    verifyTypeScriptSetup();

    // delete the cached tsconfig.json so we can get the cra-updated tsconfig
    delete require.cache[require.resolve("./tsconfig.json")];

    const tsconfig = require("./tsconfig.json");

    // restore paths
    tsconfig.compilerOptions.paths = originalTSConfig.compilerOptions.paths;

    // write tsconfig as it is written in the verifyTypeScriptSetup.js file
    fs.writeFileSync("tsconfig.json", JSON.stringify(tsconfig, null, 2).replace(/\n/g, os.EOL) + os.EOL);
}

module.exports = {
    webpack: {
        configure: config => {
            if (!config.resolve) config.resolve = {};
            if (!config.resolve.alias) config.resolve.alias = {};

            // load the ts aliases into webpack, from tsconfig
            Object.assign(config.resolve.alias, webpackCompatibleAliases);

            // do whatever else you need to

            return config;
        }
    }
}

tsconfig.json (necessary bits only)

{
  "compilerOptions": {
    "baseUrl": "./src",
    "paths": {
      "@contexts/*": [
        "contexts/*"
      ],
      "@hooks/*": [
        "hooks/*"
      ],
      "@util/*": [
        "util/*"
      ],
      "@chat-items/*": [
        "transcript/items/chat/*"
      ],
      "@transcript-items/*": [
        "transcript/items/transcript/*"
      ],
      "@devtools/*": [
        "devtools/*"
      ],
      "@transcript/*": [
        "transcript/*"
      ]
    }
  }
}

@stale
Copy link

stale bot commented Jun 26, 2021

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Jun 26, 2021
@yordis
Copy link

yordis commented Jun 27, 2021

I have a dream, bump

@stale stale bot removed the stale label Jun 27, 2021
@iansu iansu linked an issue Jun 30, 2021 that may be closed by this pull request
@hichemfantar
Copy link

Please add this already, There's no reason we have to use an external package to just add import aliases!

@pycraft114
Copy link

any recent news about this?

@EricRabil
Copy link

please

@kytosai
Copy link

kytosai commented Aug 7, 2021

This is a must-have feature

@EricRabil
Copy link

For a framework that wants to be batteries-included this is a pretty huge facepalm to leave sitting here for what could easily become a year or more.

import xyz from "../../../../contexts/xyz" vs import xyz from "@/contexts/xyz"

This seems like a pretty huge enhancement, and an easy one. The fix is sitting right here. Pleaseee.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet