Skip to content

Add support for separate type checking in Type Script #63

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

Closed
davidmpaz opened this issue Jun 30, 2017 · 3 comments
Closed

Add support for separate type checking in Type Script #63

davidmpaz opened this issue Jun 30, 2017 · 3 comments
Labels
Feature New Feature

Comments

@davidmpaz
Copy link
Contributor

According to this section we can improve significantly the compilation speed by setting transpileOnly = true, though this require using another plugin for the type checking which does it in a separate process.

Until now I have been using fork-ts-checker-webpack-plugin. I was planning to make a PR for adding it.

Today I also found ts-checker-webpack-plugin but plugin doesn't seem to be as healthy as the other one, still exists to solve some issues with the previous.

Could we discuss to bring ideas to the table?

thanks in advance.

@weaverryan
Copy link
Member

I would use whatever is officially recommended by TS, which is fork-ts-checker-webpack-plugin (since it's included in their ts-loader examples): https://github.com/TypeStrong/ts-loader

Other than that, it seems to make sense. If the user opts into this... then we can add the plugin :). To opt in... it seems like we're taking Encore down this path of having Encore-specific options after the callback:

.enableTypeScriptLoader(function() {}, {
    fork_ts_checker_plugin: true
});

As perfectly consistent as that is, other ideas are welcome :).

@weaverryan weaverryan added the Feature New Feature label Jul 2, 2017
@davidmpaz
Copy link
Contributor Author

Which plugin to use is then clear. Regarding implementation I had in mind 2 solutions:

  1. Add the plugin automagically when user configure ts-loader with transpileOnly: true. Options would have sensible defaults.
  2. Add explicit API for enableForkedTypesChecking(pluginConfig). This would enforce settings in ts-loaders for instance: transpileOnly: true. As well as any validation needed.

My point .1 I think would be problematic the same as your proposed solution in the way that plugin configuration will be hard (point 1) or we will be duplicating options on the way (Encore-specific options after the callback). Still would be the easiest way to use out of the box.

Point 2 add more verbosity and allow for more plugin configuration later on. Calling the API would store options and flag and when configuring ts-loader plugin would be added.

@weaverryan
Copy link
Member

I like option 2!

weaverryan added a commit that referenced this issue Jul 25, 2017
This PR was squashed before being merged into the master branch (closes #99).

Discussion
----------

Refactor plugin configuration

Extract encore built in plugin configuration logic into plugin utils.

First step toward allowing to be able to configure plugins through options and have more modular integration for plugins inside encore. From now on only public API with proper options is needed,
for most cases.

This changes are internals and tries to improve plugin architecture inside encore. Please take a look on it and feedback are welcome :). So far I see improvements for several issues:

* #2 - Point .2 ...for plugins we should do the same as for loaders, from point of view of module split (utilities).
* #63 - Better (more modular) integration of the plugin as well its configuration.
* #65 - Same as #63
* #79 - Now it should be possible to pass options and/or check for flags to apply plugin or not.
* #87 - Some small steps to make clean plugin more configurable. Public API still need to be changed though.

thanks in advance.

Commits
-------

65adfdc Refactor plugin configuration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants