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

Add compilerOptions option #173

Merged
merged 4 commits into from
Oct 24, 2018

Conversation

ianschmitz
Copy link
Contributor

@ianschmitz ianschmitz commented Oct 23, 2018

Adds compilerOptions option, using ts-loader as inspiration. This allows overriding compilerOptions of your tsconfig.json explicitly when initializing the plugin.

Here it is in action using create-react-app with the newly added TypeScript support:

image

Here is the discussion over at CRA for the background on this feature: facebook/create-react-app#5514

TODO:

  • Unit/Integration tests?
  • Validate Vue works as expected

/cc @Timer, @brunolemos

@ianschmitz
Copy link
Contributor Author

@johnnyreilly see facebook/create-react-app#5514 (comment). Do you think this PR still has value?

@johnnyreilly
Copy link
Member

I think the PR still has value; it's another option that people can use - it's been useful with ts-loader. Always considered it an escape hatch 😄

A test to cover this would be awesome!

@ianschmitz
Copy link
Contributor Author

Unit tests should be good to go now

@johnnyreilly
Copy link
Member

Great! Could you increment the version in the package.json and put an entry in the changelog.md please? It makes shipping a release much easier for me!

@ianschmitz
Copy link
Contributor Author

👍

Copy link
Member

@johnnyreilly johnnyreilly left a comment

Choose a reason for hiding this comment

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

Could you take a look at the comments below please?

@@ -60,6 +60,9 @@ It helps to distinguish lints from typescript's diagnostics.
* **tsconfig** `string`:
Path to *tsconfig.json* file. Default: `path.resolve(compiler.options.context, './tsconfig.json')`.

* **compilerOptions** `string`:
Allows overriding TypeScript options. Should be specified in the same format as you would do for the `compilerOptions` property in tsconfig.json. Default: `{}`.

* **tslint** `string | true`:
Path to *tslint.json* file or `true`. If `true`, uses `path.resolve(compiler.options.context, './tslint.json')`. Default: `undefined`.
Copy link
Member

Choose a reason for hiding this comment

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

Should the documented type of compilerOptions be object instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right. Good catch! Was a late night for me yesterday 😛

README.md Outdated Show resolved Hide resolved
Co-Authored-By: ianschmitz <ianschmitz@gmail.com>
// 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)
);

parsed.options = { ...parsed.options, ...compilerOptions };
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we can use object spread as it won't work on node 6 (and fork-ts-checker supports node 6). https://node.green/#ES2018-features-object-rest-spread-properties-object-spread-properties

Can we switch to Object.assign please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typescript will compile it down to Object.assign for you as long as your target is correct. It's a little tough for me to check as I'm on my phone right now but will double check tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you use assign elsewhere in code base I don't mind changing either way!

Copy link
Member

Choose a reason for hiding this comment

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

Typescript will compile it down to Object.assign for you as long as your target is correct. It's a little tough for me to check as I'm on my phone right now but will double check tomorrow.

If that's the case then that's fine with me!

@@ -40,6 +40,7 @@ export class VueProgram {
);

parsed.options.allowNonTsExtensions = true;
parsed.options = { ...parsed.options, ...compilerOptions };
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we can use object spread as it won't work on node 6 (and fork-ts-checker supports node 6). https://node.green/#ES2018-features-object-rest-spread-properties-object-spread-properties

Can we switch to Object.assign please?

Copy link
Member

Choose a reason for hiding this comment

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

Again - fine with this staying as is if TypeScript transpiles us to glory! 😄

@Timer
Copy link

Timer commented Oct 24, 2018

Having this for the exclude property would be 💯.

@ianschmitz
Copy link
Contributor Author

Having this for the exclude property would be 💯.

@Timer unfortunately this doesn't work for exclude 😞 . See facebook/create-react-app#5514 (comment)

@Timer
Copy link

Timer commented Oct 24, 2018

I meant as a separate change. Is there another way to override this value, i.e. a new PR? Or is it not possible?

@ianschmitz
Copy link
Contributor Author

Is there another way to override this value, though?

This looks interesting: https://github.com/TypeStrong/ts-loader#reportfiles-string-default. The implementation can be found here: https://github.com/TypeStrong/ts-loader/blob/35b9a5bbf298b5ea4e5b32057dbf252337b9fd08/src/utils.ts#L71

Looks like they attach to the TS diagnostics and filter out errors if the files aren't part of the reportFiles globs.

@Timer
Copy link

Timer commented Oct 24, 2018

Ooh, that'd be fantastic to implement. I don't want to ship excludes with CRA (in the tsconfig -- I'd way rather silently filter out tests).

@ianschmitz
Copy link
Contributor Author

Ooh, that'd be fantastic to implement. I don't want to ship excludes with CRA.

Agreed!

@johnnyreilly what do you think about this? I haven't dug too deep into this repo to know how easy it would be to hook the after compile diagnostics.

@Timer
Copy link

Timer commented Oct 24, 2018

https://github.com/Realytics/fork-ts-checker-webpack-plugin/blob/f57ece232510f490841682d71ddad9bae0822e18/src/index.ts#L713-L760

Here's some usage, but it's littered over quite a few places. Not sure where it'd be best to filter this.

@johnnyreilly
Copy link
Member

@johnnyreilly what do you think about this?

Worth investigating - if it's straightforward then do it!

@Timer
Copy link

Timer commented Oct 24, 2018

@johnnyreilly could you suggest a location to perform the filtering at?

In the workers themselves, at where the worker aggregates, somewhere else? Can you please point to a file or some lines? 😄

@johnnyreilly johnnyreilly merged commit 9e408c5 into TypeStrong:master Oct 24, 2018
@Timer
Copy link

Timer commented Oct 24, 2018

Also, in relation to this PR:
For an option, e.g. module, do you need to pass ts.ModuleKind.ESNext or "esnext"?

@johnnyreilly
Copy link
Member

Merged - thought it was good to take as is. Figure that the reportFiles merits it's own PR 😄

@ianschmitz
Copy link
Contributor Author

ianschmitz commented Oct 24, 2018

Also, in relation to this PR:
For an option, e.g. module, do you need to pass ts.ModuleKind.ESNext or "esnext"?

Very good point @Timer. I think that may be a deficiency. Just looking through https://github.com/TypeStrong/ts-loader/blob/d49dfbaa7e35c728090554d47a122ad2571cb09c/src/config.ts#L66, they apply the transformation before running it through ts.parseJsonConfigFileContent(). Should be an easy change in this repo as we just need to break out the readConfigFile() into a separate variable and merge in the options.

To clarify, we should not require the user to provide the value of ts.ModuleKind.ESNext imo. "esnext" should be sufficient, but that would require modifying the result of readConfigFile().

Sorry about that!

@ianschmitz
Copy link
Contributor Author

@johnnyreilly do you want to hold off publishing until we merge in the fix?

@ianschmitz ianschmitz deleted the add-compileroptions-option branch October 24, 2018 21:59
@johnnyreilly
Copy link
Member

@ianschmitz - too late! https://github.com/Realytics/fork-ts-checker-webpack-plugin/releases/tag/v0.4.12

Still, it's an extra option which people won't be using yet so no harm done - if you could submit a PR for the fix that'd be appreciated!

@johnnyreilly
Copy link
Member

@johnnyreilly could you suggest a location to perform the filtering at?

In the workers themselves, at where the worker aggregates, somewhere else? Can you please point to a file or some lines? 😄

@Timer - I don't have a strong opinion. I have a feeling that experimenting during implementation should be a good guide. I'd expect this to be a fairly simple tweak, not requiring of masses of code; if you find yourself writing War and Peace then you might want to reconsider where you're doing the filtering 😄

Essentially, go with your gut.

@ianschmitz
Copy link
Contributor Author

@johnnyreilly no problem. I'll have a PR up tomorrow with the fix!

@johnnyreilly
Copy link
Member

Awesome!

@johnnyreilly
Copy link
Member

@Timer - sorry about the delay in replying!

The simplest implementation would seem to be taking this approach here:

https://github.com/TypeStrong/ts-loader/blob/d49dfbaa7e35c728090554d47a122ad2571cb09c/src/utils.ts#L54-L75

and adding a filter in before the forEach here:

https://github.com/Realytics/fork-ts-checker-webpack-plugin/blob/458cd748519c04501298c342de31acf1b2f84336/src/index.ts#L741

This would be handling it in the worker aggregate position. It's possible that there might be a minor performance benefit to doing this in the worker itself but I'm not sure how big the gain would be. For now, I'd take simplicity.

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.

3 participants