Absolute paths using REACT_APP_NODE_PATH instead of NODE_PATH

[smacpherson64]

Closing PR as it is does not resolve issue

Aimed to fix #.122

Creating concept to utilize REACT_APP_ variables in .env files to solve the issue of Absolute paths.

[wmonk]

@eonwhite @DorianGrey could you have a look over this?

[DorianGrey]

I'm not sure about the advantage of renaming NODE_PATH to REACT_APP_NODE_PATH, though. NODE_PATH is handled properly using webpack and jest, and tsc simply ignores it, so it should not conflict with anything else.
In general, the idea of using baseUrl should work, at least for a single folder to look non-relative linked modules up from. It will require a bit of documentation, though - while .env files and thus NODE_PATH might include multiple paths, baseUrl will only be able to deal with a single one automatically, additional mappings require the paths definition. It might be useful to add an additional warning or possibly an error if multiple paths are defined using the .env strategy. Alternatively, it should be possible to provide a check if the NODE_PATH entries have matching counterparts defined in the tsconfig.json, and only throw an error in that case.

[smacpherson64]

Hey @DorianGrey @wmonk, Thank you for responding!

I apologize for the background. About the switching from NODE_PATH to REACT_APP_NODE_PATH, I am using react-scripts-ts version 2.8.0 on Windows 10 and am unable to use NODE_PATH as it returns undefined in the following situations:

.env files
NODE_PATH=src

In package.json scripts:
set NODE_PATH=src&&...

I tried variants, but NODE_PATH consistently returned undefined. Other variables were working as desired. After reading #.122, I assumed that this was a similar problem to my own. This is where I may have missed the point from @DorianGrey:

As you recognized correctly, typescript does not support "NODE_PATH" (using it is discouraged in general, from what I know) - instead, you have to go with path mapping, as suggested here:

I interpreted this as the same issue when they were not.

[smacpherson64]

Again, apologies, going to remove all traces of responses in #.122 to reduce confusion. I ran through tests again, and NODE_PATH is working as desired (for webpack and jest) as you said above.

Actions taken:

• Creating a .env file in project root with:
  NODE_PATH=src

• Adding baseUrl option in tsconfig.json
"baseUrl": "src"

• Setup Absolute Import
  import { Component } from components/Component

It is thus unnecessary to rename NODE_PATH for my setup (Windows 10, Node 8.7.0, react-scripts-ts 2.8.0). Note: I didn't need to set up paths in tsconfig.json yet, as I only used one path and was able to just use baseUrl.

Again, Apologies, thanks for your time!

[smacpherson64]

@DorianGrey here is a concept that may work in env.js after the NODE_PATH declaration (line 58)

It might be useful to add an additional warning or possibly an error if multiple paths are defined using the .env strategy. Alternatively, it should be possible to provide a check if the NODE_PATH entries have matching counterparts defined in the tsconfig.json, and only throw an error in that case.

 if (!!process.env.NODE_PATH) {
   const tsconfig = JSON.parse(fs.readFileSync(paths.appTsConfig, 'utf8'));
   const hasMultiplePaths = process.env.NODE_PATH.indexOf(path.delimiter) !== -1;
   const hasBaseUrl = typeof tsconfig.compilerOptions.baseUrl !== 'undefined';
   const hasPaths = typeof tsconfig.compilerOptions.paths !== 'undefined';
 
   if (hasMultiplePaths && !(hasBaseUrl && hasPaths)) {
     throw new Error(
       `Setting multiple paths using the NODE_PATH env variable requires adding baseUrl and paths to compilerOptions in tsconfig.json. For more information on how to setup paths in tsconfig.json visit: https://www.typescriptlang.org/docs/handbook/module-resolution.html#path-mapping
       
       `
     );
   }
 
   if (!hasBaseUrl) {
     throw new Error(
       `Setting a path using the NODE_PATH env variable requires adding baseUrl to compilerOptions in tsconfig.json. For more information on how to setup baseUrl visit: https://www.typescriptlang.org/docs/handbook/module-resolution.html#base-url
       
       `
     );
   }
 }

[DorianGrey]

Just a short hint for

In package.json scripts:
set NODE_PATH=src&&...

That does not work really well in most terminals on windows when npm or yarn are involved. In cases where this kind of setting environment variables is required, I've always used cross-env to get it working in a cross-platform way.
cross-env NODE_PATH=src <your command>
Using the native set on windows under these circumstances always behaves a bit ... odd. Sometimes more than just a bit. So this is a general problem that CRA suffers from as well, unless used properly.

Regarding the checks: The seconds check looks good. However, there is a caveat on the first check:

if (hasMultiplePaths && !(hasBaseUrl && hasPaths)) {
     throw new Error(
       `Setting multiple paths using the NODE_PATH env variable requires adding baseUrl and paths to compilerOptions in tsconfig.json. For more information on how to setup paths in tsconfig.json visit: https://www.typescriptlang.org/docs/handbook/module-resolution.html#path-mapping
       `
     );
   }

This one isn't precise enough - it would only check for an amount, not for matching paths.
E.g. using a "baseUrl": "src" and

"paths": {
    "example/*": "common/whatever/*"
}

it should be ensured that all paths included by NODE_PATH should be listed as keys here (but beware of skipping the wildcard), in this case example. If NODE_PATH would include example2 instead of example, the check should not pass, but it would in the current state.
Setting up a check like this is not a trivial task, though - however, I'd say it's worth the time, since failing on proper configuration is too easy here without guidance.

[smacpherson64]

Hey @DorianGrey, I am going to put examples below to see if I am understanding these correctly. If you have time please let me know your thoughts!

Assumptions

• All paths are based upon baseUrl (baseUrl + paths)
• Path Matches' key is the match to test the import against
• Path Matches' value (string[] or string) is/are the location(s) to use when searching for the module.

Examples

Assuming the following import on the examples:

import 'src/common/components/Test'

Example 1 - Match anything with two search paths (glob & generated/glob)

tsconfig.json

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

Result:
Matches and looks for the module in the following location(s):
./src/common/components/Test
./generated/src/common/components/Test

Example 2 - Match src folder

tsconfig.json

...
"baseUrl": ".",
"paths": {
  "src/*": "src/*",
},

Result:
Matches and looks for the module in the following location(s):
./src/common/components/Test

Example 3 - Match common folder with src as baseUrl

tsconfig.json

...
"baseUrl": "src",
"paths": {
  "common/*": "common/*",
},

Result:
Matches and looks for the module:
./src/common/components/Test

Example 4 - Multiple matches with baseUrl as src

tsconfig.json

...
"baseUrl": "src",
"paths": {
  "another/*": "another/*"
  "common/*": "common/*",
},

Result:
The first match fails, The second match succeeds and looks for the module in the following location(s):
./src/common/components/Test

[DorianGrey]

Examples 2, 3 and 4 should be correct, from what I can see.
If I got that in mind correctly, the first example won't work, i.e. you'd get a compiler error here - path mappings have to include paths that are not only wildcards.

[smacpherson64]

Hey @DorianGrey,

Sorry for taking so long to respond, Example 1 was from the configs on the typescript site above wasn't sure about that.

Here is round two, this would conceptually be placed around line 47 on react-scripts-ts/config/env.js. before the setting of process.env.NODE_PATH =(process.env.NODE_PATH || {})

/**
 * Determine if tsconfig.json and NODE_ENV contain the same configuration with paths.
 * 
 * @param {*} tsconfig compilerOptions
 * @param {*} The process.env.NODE_ENV variable
 */
function validatePathsMatchBetweenConfigs(compilerOptions, nodePaths) {
  const tsPaths = compilerOptions.paths;

  const mismatchedPaths = nodePaths
    .split(path.delimiter)
    .reduce(
      (unmatchedPaths, nodePath) => unmatchedPaths.filter(tsPath => tsPath !== nodePath),
      Object.keys(tsPaths).map(key => `${compilerOptions.baseUrl}/${tsPaths[key]}`)
    );

  return mismatchedPaths.length === 0;
}

if (!!process.env.NODE_PATH) {
  const tsconfig = JSON.parse(fs.readFileSync(paths.appTsConfig, 'utf8'));
  const options = tsconfig.compilerOptions;
  const nodePath = process.env.NODE_PATH;
  const hasMultiplePaths = nodePath.indexOf(path.delimiter) !== -1;
  const hasBaseUrl = typeof options.baseUrl !== 'undefined';
  const hasPaths = typeof options.paths !== 'undefined';

  if (hasMultiplePaths) {
    if (!(hasBaseUrl && hasPaths)) {
      throw new Error(
        `Setting multiple paths using the NODE_PATH env variable requires adding baseUrl and paths to compilerOptions in tsconfig.json. For more information on how to setup paths in tsconfig.json visit: https://www.typescriptlang.org/docs/handbook/module-resolution.html#path-mapping
          
        `
      );
    }

    const pathsAreInSync = validatePathsMatchBetweenConfigs(options, nodePath);

    if (!pathsAreInSync) {
      throw new Error(
        `A mismatch exists between NODE_PATH env variable and paths set in the compilerOptions in tsconfig.json. For more information on how to setup paths and baseUrl visit: https://www.typescriptlang.org/docs/handbook/module-resolution.html#base-url
            
        `
      );
    }
  }

  if (!hasBaseUrl) {
    throw new Error(
      `Setting a path using the NODE_PATH env variable requires adding baseUrl to compilerOptions in tsconfig.json. For more information on how to setup baseUrl visit: https://www.typescriptlang.org/docs/handbook/module-resolution.html#base-url
         
      `
    );
  }
}

[DorianGrey]

There are several caveats with this approach.

1. Reading the typescript compiler options is a bit more complicated than just picking up a json file, at least in the general case. This is caused by the extends clause. I've once implemented this for the ts-jest project here.
2. The if (!(hasBaseUrl && hasPaths) condition has to be used even if there is just one path in nodePath, not only in case of multiple paths.
3. The path comparison has to be performed for just a single path provided by NODE_ENV as well.
4. The path comparison has to ignore potentially existing glob wildcards and trailing slashes.

Never mentioned this would by easy ;)
Considering the amount of effort this comparison takes and the potential misbehaviors (i.e. failed or inaccurate checks) that may occur, I'd recommend to not break with an error in every potential case, just in the basic ones like if there is at least a single path in NODE_ENV, there has to be a baseUrl in the tsconfig.

[smacpherson64]

Thank you again for your time @DorianGrey!

Agreed on the full matching it seems that any match checking would end up being brittle in nature. (2, 3, and 4).

1. For basic path matching we would need to handle the extends clause like you have done for tslint.
   I had a fun looking through your implementation above, Provide an API to convert tsconfig.json to CompilerOptions microsoft/TypeScript#5276 and https://github.com/Microsoft/TypeScript/blob/v2.5.3/src/compiler/commandLineParser.ts.

Below in the gist I reworked the concept (moving towards external file) using the direct port of what you used for tslint. I added a concept in, if the system does throw an error we could put an example (clearly marked as an example partial) so that users could get a quick idea of a possible way to use baseUrl and paths:

Multiple Paths

Error: Setting multiple paths using the NODE_PATH env variable requires adding baseUrl and paths to compilerOptions in tsconfig.json. For more information on how to setup paths in tsconfig.json visit: https://www.typescriptlang.org/docs/handbook/module-resolution.html#path-mapping

Below is a partial tsconfig.json example displaying baseUrl and paths usage:

{
  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "src": "src",
      "test": "test"
    }
  }
}

Single Path

Error: Setting a path using the NODE_PATH env variable requires adding baseUrl to compilerOptions in tsconfig.json. For more information on how to setup baseUrl visit: https://www.typescriptlang.org/docs/handbook/module-resolution.html#base-url

Below is a partial tsconfig.json example displaying baseUrl usage:

{
  "compilerOptions": {
    "baseUrl": "src"
  }
}

Again appreciate your thoughts and time!

https://gist.github.com/smacpherson64/425d8003d9033d83f85e445b8c5fbb61

[DorianGrey]

That looks good, aside from some (minor) code issues:

• getStartDir seems to be unused ... don't think we'll need this ;) And even if it is required to pick up some paths, the paths module should be used for this, since it also handles potentially different resolution with or without ejection.
• This line seems curious - the variable you're referring to (tsconfig) does not exist.
• The displayed examples are a bit trivial - I'd prefer something more realistic, like:

{
  "compilerOptions": {
    "baseUrl": ".",
    "paths": {
      "shared/*": "src/shared/*",
      "someFeature/*": "src/someFeature/*"
    }
  }
}

A more detailed comparison of the paths would be favorable, but I'm afraid there is quite a huge potential for false positives during this evaluation. Suppose it's better to leave it cut off as it is in your current implementation. Might add some improvements here in case we find a robust way to implement such checks.

Thinking about this a little more, it comes to my mind that process.env.NODE_PATH may potentially contain arbitrary paths - they do not necessarily have to be linked to the project. We should only complain about paths that affect the current project. Including files outside of the current project is already blocked by default (by the ModuleScopePlugin).

Something like:

const paths = require('./paths');
// ...
const currentPath = paths.resolveApp();
const nodePaths = nodePathEnv
  .split(path.delimiter)
  .filter(p => p.indexOf(currentPath) >= 0)
;
if (nodePaths.length > 0) {
  // The rest of the checks
}