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

Merge in compilerOptions prior to calling parseJsonConfigFileContent #176

Merged
merged 2 commits into from
Oct 27, 2018

Conversation

ianschmitz
Copy link
Contributor

@ianschmitz ianschmitz commented Oct 26, 2018

This PR fixes a deficiency in #173 (comment), as we were merging the compilerOptions in too late in the pipeline, which meant that if someone wanted to override a configuration option that was a string value example (such as module or target), they would have to use the typescript enum integer value instead (module: 1 instead of module: "es5").

Here is an example where I'm overriding in the webpack config of create-react-app. My tsconfig.json specifies "moduleResolution": "node", but i override it to "classic" here:
image

/cc @Timer

@ianschmitz
Copy link
Contributor Author

Note that previously we were merging in isolatedModules: "false" into the tsconfig, but from as best as i can tell that had no effect as it wasn't being merged in to the config.compilerOptions.

To prevent a breaking change I simply removed this logic

@johnnyreilly
Copy link
Member

johnnyreilly commented Oct 26, 2018

To prevent a breaking change I simply removed this logic

I've just been looking back on the history; this method used to look something like this:

static loadProgramConfig(configFile: string) {
    return ts.parseJsonConfigFileContent(
      // Regardless of the setting in the tsconfig.json we want isolatedModules to be false
      Object.assign(ts.readConfigFile(configFile, ts.sys.readFile).config, { isolatedModules: false }),
      ts.sys,
      path.dirname(configFile)
    );
  }

I believe that this compiler flag is significant ; @piotr-oles can you advise?

I'm uncomfortable with this coming out to be honest.

Still evidence is king. It would be good to take a slightly older version of fork-ts-checker-webpack-plugin, hack in a console.log to the loadProgramConfig to determine whether isolatedModules: "false" used to be set and we were just mistaken. Totally possible!

@ianschmitz
Copy link
Contributor Author

The output of ts.readConfigFile(configFile, ts.sys.readFile).config is the raw tsconfig.json, which would have compilerOptions, include, exclude, etc. I'm not sure how that would have worked previously 😕.

Regardless i can try an older version and do as you suggested tonight!

@johnnyreilly
Copy link
Member

johnnyreilly commented Oct 26, 2018

Regardless i can try an older version and do as you suggested tonight!

If you could I'd appreciate it!

cc @piotr-oles

It's possible that this wasn't picked up as the default value of isolatedModules is false...

https://www.typescriptlang.org/docs/handbook/compiler-options.html

Huh... I had completely forgotten I was the cause of all this in the first place!

See: #17

And: #18

@johnnyreilly
Copy link
Member

Okay - having reread all that (isn't it amazing what you forget?!) I'm coming to the conclusion that losing this from the codebase is just fine I think.

If you could still do that check I'd appreciate it. Odds on I'd say this should be good to merge.

@ianschmitz
Copy link
Contributor Author

Alright here's the results @johnnyreilly:

I added in a few console.log() entries on 0.4.10. From what i can see the output is what we expected. The isolatedModules: "false" is up a level too high, and should be inside the compilerOptions to have an effect:

output from ts.readConfigFile() isolatedModules hasnt been applied yet:

{ extends: './tsconfig.test.json',
  compileOnSave: false,
  compilerOptions:
   { target: 'es5',
     allowJs: true,
     skipLibCheck: true,
     esModuleInterop: true,
     allowSyntheticDefaultImports: true,
     strict: true,
     moduleResolution: 'node',
     resolveJsonModule: true,
     isolatedModules: true, <=======
     noEmit: true,
     jsx: 'preserve' } }


output from ts.parseJsonConfigFileContent() note the 'raw' property:

{ options:
   { module: 6,
     target: 1,
     allowJs: true,
     skipLibCheck: true,
     esModuleInterop: true,
     allowSyntheticDefaultImports: true,
     strict: true,
     moduleResolution: 2,
     resolveJsonModule: true,
     isolatedModules: true, <=======
     noEmit: true,
     jsx: 1,
     configFilePath: undefined },
  fileNames:
   [ 'C:/Projects/create-react-app/packages/react-scripts/template/src/App.js',
     'C:/Projects/create-react-app/packages/react-scripts/template/src/index.tsx',
     'C:/Projects/create-react-app/packages/react-scripts/template/src/react-app.d.ts',
     'C:/Projects/create-react-app/packages/react-scripts/template/src/serviceWorker.d.ts',
     'C:/Projects/create-react-app/packages/react-scripts/template/src/serviceWorker.js' ],
  projectReferences: undefined,
  typeAcquisition: { enable: false, include: [], exclude: [] },
  raw:
   { extends: './tsconfig.test.json',
     compileOnSave: false,
     compilerOptions:
      { target: 'es5',
        allowJs: true,
        skipLibCheck: true,
        esModuleInterop: true,
        allowSyntheticDefaultImports: true,
        strict: true,
        moduleResolution: 'node',
        resolveJsonModule: true,
        isolatedModules: true, <=======
        noEmit: true,
        jsx: 'preserve' },
     isolatedModules: false, <=======
     include: [ 'src' ],
     exclude: [ '**/__tests__/**', '**/?*test.*', '**/?*spec.*', 'foo' ] },
  errors: [],
  wildcardDirectories:
   { 'C:/Projects/create-react-app/packages/react-scripts/template/src': 1 },
  compileOnSave: false,
  configFileSpecs:
   { filesSpecs: undefined,
     includeSpecs: [ 'src' ],
     excludeSpecs: [ '**/__tests__/**', '**/?*test.*', '**/?*spec.*', 'foo' ],
     validatedIncludeSpecs: [ 'src' ],
     validatedExcludeSpecs: [ '**/__tests__/**', '**/?*test.*', '**/?*spec.*', 'foo' ],
     wildcardDirectories:
      { 'C:/Projects/create-react-app/packages/react-scripts/template/src': 1 } } }

@ianschmitz
Copy link
Contributor Author

Would also explain this comment: #17 (comment)

@johnnyreilly
Copy link
Member

Thanks @ianschmitz - I'm convinced! 😁

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 this pull request may close these issues.

2 participants