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

Extension configuration should override, not deep merge #94

Closed
OliverJAsh opened this issue Sep 10, 2019 · 4 comments · Fixed by #95
Closed

Extension configuration should override, not deep merge #94

OliverJAsh opened this issue Sep 10, 2019 · 4 comments · Fixed by #95

Comments

@OliverJAsh
Copy link
Contributor

OliverJAsh commented Sep 10, 2019

  • Version: 3.8.0
  • keywords: inherit, inherited, inheritance, extended, extends

loadTsConfig performs a deep merge on extended configurations:

return deepmerge(base, config);

… but this is not what TypeScript itself does when resolving extended configurations:

The configuration from the base file are loaded first, then overridden by those in the inheriting config file. If a circularity is encountered, we report an error.

https://www.typescriptlang.org/docs/handbook/tsconfig-json.html#configuration-inheritance-with-extends

This manifested in a very confusing bug for us, whereby we were using tsconfig-paths-webpack-plugin (which uses this module) and module paths were being resolved to the wrong place.

Reduced test case:

./shared/tsconfig.json:

{
    "compilerOptions": {
        "strict": true,

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

./src/tsconfig.json:

{
    "extends": "../shared/tsconfig.json",
    "compilerOptions": {
        "baseUrl": ".",
        "paths": {
            "shared/*": ["../shared/*"]
        }
    }
}
$ tsc --showConfig --project ./src/
{
    "compilerOptions": {
        "strict": true,
        "baseUrl": "./",
        "paths": {
            "shared/*": [
                "../shared/*"
            ]
        }
    },
    "files": [
        "./main.ts"
    ]
}
node
> require('tsconfig-paths').loadConfig('./src/')
{ resultType: 'success',
  configFileAbsolutePath:
   '/Users/oliverash/Development/typescript-playground/src/tsconfig.json',
  baseUrl: '.',
  absoluteBaseUrl: '/Users/oliverash/Development/typescript-playground/src',
  paths: { 'shared/*': [ './*', '../shared/*' ] } }

Observe how tsc only uses the paths from ./src/tsconfig.json, whereas tsconfig-paths uses paths from both ./src/tsconfig.json and ./shared/tsconfig.json.

Actual vs expected:

-   paths: { 'shared/*': [ './*', '../shared/*' ] } }
+   paths: { 'shared/*': [ '../shared/*' ] } }

Now if you use a module path of shared/foo, webpack will incorrectly use the file ./src/foo.ts whereas TS (tsc) will correctly use the file ./shared/foo.ts.

const TsConfigPaths = require("tsconfig-paths");

const config = TsConfigPaths.loadConfig("./src/");
const matchPath = TsConfigPaths.createMatchPath(
  config.absoluteBaseUrl,
  config.paths,
  config.mainFields,
  config.addMatchAll
);
matchPath("shared/foo", console.log); // ./src/foo/package.json 🔴 

For this reason I would consider it a bug that this module uses a deep merge. Would you accept a PR which fixes this to match TypeScript's own behaviour (that is, override instead of merge)?

@jonaskello
Copy link
Member

If we're not following the behaviour of tsc I definitely agree it is a bug and a PR would be accepted to fix it.

@OliverJAsh OliverJAsh changed the title Extended configuration should override, not deep merge Extension configuration should override, not deep merge Sep 11, 2019
OliverJAsh added a commit to OliverJAsh/tsconfig-paths that referenced this issue Sep 11, 2019
OliverJAsh added a commit to OliverJAsh/tsconfig-paths that referenced this issue Sep 11, 2019
@OliverJAsh
Copy link
Contributor Author

#95

jonaskello pushed a commit that referenced this issue Sep 12, 2019
* yarn-deduplicate && yarn

* Make extension config override instead of deep merge

Fixes #94
@VeeteshJain
Copy link

@OliverJAsh this change is causing microsoft/TypeScript#20110 (comment) issue with nx monorepo.
can we have both the options one to extend and another to deepmerged based on the different use cases?

@OliverJAsh
Copy link
Contributor Author

@VeeteshJain This package just matches the built-in TypeScript behaviour. I don't think we should differ from that?

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

Successfully merging a pull request may close this issue.

3 participants