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

feat(Provide better API and Exposure to compiler-cli codegen and transpile) #8759

Closed
TheLarkInn opened this issue May 20, 2016 · 24 comments
Closed
Assignees
Labels
area: core Issues related to the framework runtime feature Issue that requests a new feature

Comments

@TheLarkInn
Copy link
Member

I have been charged with the task of writing a webpack loader for the CompilerCLI's offline compilation a code generation feature.

Because the compiler-cli was written as a one off 'build step', it implements ts.CompilerHost. This however poses a problem for Lifetime transpilation tools like a webpack loader. If you look at the source code in both ts-loader and awesome-typescript-loader you'll notice that both of these implement ts.LanguageServiceHost. Although these two interfaces overlap, piping a ts.LanguageServiceHost is not a plug-and-play solution: TheLarkInn/awesome-typescript-loader@7b21b13

Proposal

  1. Expose any of the functionality required in the compiler-cli (codegen, annotation conversions) so that the those steps can be performed on a per-file basis. Since webpack already handles finding the correct .ts files via loaders any of that boilerplate code that is used thanks to ts.CompilerHost to assist in finding and reading files is obsolete/not needed.
  2. Allow any of the currently exposed classes from the compiler-cli (TsickleHost, Codegenerator, ReflectionHost, MetadataHost) to accept and incorporate ts.LanguageServiceHost.
@TheLarkInn
Copy link
Member Author

@s-panferov @jbrantly I'd love to not fork and implement something like this from your repo's, do you think when this issue is implemented we could talk about incorporating a plugin into your typescript loaders that incorporate the extra angular offline compiler functionality?

@s-panferov
Copy link

@TheLarkInn I've already had some thoughts about code-gen plugin support, so I definitely approve an idea to add some external plugin support.

Feel free to create a corresponding issue in awesome-typescript-loader issue tracker.

@TheLarkInn
Copy link
Member Author

@tbosch here's a rough example of what I'm referring to:

TheLarkInn@45350bb

TheLarkInn referenced this issue May 27, 2016
This allows angular's build to depend on some extensions, but not on code generation, and breaks a cycle in the angular build
We now merge ts-metadata-collector into tsc-wrapped and stop publishing the former.
@IgorMinar
Copy link
Contributor

IgorMinar commented Jun 13, 2016

@alexeagle can you please chime in with your plans to provide incremental compilation story?

@alexeagle
Copy link
Contributor

As discussed elsewhere, we're confident that CompilerHost is the right API, regardless of whether the compilation is in a single pass.
Indeed, tsc-wrapped already does more than one pass - we create an initial program, then use it for codegen, add the generated files to a new program, then type-check that, then change the compiler host for tsickle and create yet another program.

I am switching back to google-internal work right now, so I won't have time to add the --watch mode to ngc yet. But other than serving as an example for loaders, I don't think you depend on that.

@TheLarkInn
Copy link
Member Author

I can take a stab at a PR. Are we against the feature as an idea? Or is it a matter of not being a priority to implement.

@alexeagle
Copy link
Contributor

We're not against ngc --watch, seems like an obvious thing we need to
provide if we are wrapping the compiler.
I think a good PR would look at the APIs used by tsc and do something as
similar as possible, so we stay compatible in all the edge cases.

On Mon, Jun 13, 2016 at 10:12 AM Sean Larkin notifications@github.com
wrote:

I can take a stab at a PR. Are we against the feature as an idea? Or is it
a matter of not being a priority to implement.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8759 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAC5I4zcJ2DkLj6OcQZHrnIkmQW8eU5Uks5qLY-FgaJpZM4IjoJn
.

@TheLarkInn
Copy link
Member Author

TheLarkInn commented Jun 13, 2016

@alexeagle @chuckjaz I'm fine with that. Watch mode looks easy enough to implement.

But referencing here:
https://github.com/Microsoft/TypeScript/blob/master/src/compiler/tsc.ts#L527-L537

function directoryChangeHandler() {
            const parsedCommandLine = parseConfigFile();
            const newFileNames = ts.map(parsedCommandLine.fileNames, compilerHost.getCanonicalFileName);
            const canonicalRootFileNames = ts.map(rootFileNames, compilerHost.getCanonicalFileName);

            // We check if the project file list has changed. If so, we just throw away the old program and start fresh.
            if (!arrayIsEqualTo(newFileNames && newFileNames.sort(), canonicalRootFileNames && canonicalRootFileNames.sort())) {
                setCachedProgram(undefined);
                startTimerForRecompilation();
            }
        }

If I read this correctly, (and then the code it calls), it looks like when files are changed, that there isn't even CompilerHost based incremental support. (Please correct me if I'm wrong). So users of a very scaled angular application would see slow build times, (even in watch mode).

Almost like trying to compile the angular/angular tsc src every time a file changes. 👎 Is this the path we want to take?

@alexeagle
Copy link
Contributor

I think that code is handling the case that your list of files[] in
tsconfig.json has changed.
Note that we already use tsc --watch for large projects like angular. The
re-typechecking the world is the slow part, not reconstructing the state in
tsc.

On Mon, Jun 13, 2016 at 3:39 PM Sean Larkin notifications@github.com
wrote:

@alexeagle https://github.com/alexeagle @chuckjaz
https://github.com/chuckjaz I'm fine with that. Watch mode looks easy
enough to implement.

But referencing here:

https://github.com/Microsoft/TypeScript/blob/master/src/compiler/tsc.ts#L527-L537

function directoryChangeHandler() {
const parsedCommandLine = parseConfigFile();
const newFileNames = ts.map(parsedCommandLine.fileNames, compilerHost.getCanonicalFileName);
const canonicalRootFileNames = ts.map(rootFileNames, compilerHost.getCanonicalFileName);

        // We check if the project file list has changed. If so, we just throw away the old program and start fresh.
        if (!arrayIsEqualTo(newFileNames && newFileNames.sort(), canonicalRootFileNames && canonicalRootFileNames.sort())) {
            setCachedProgram(undefined);
            startTimerForRecompilation();
        }
    }

If I read this correctly, (and then the code it calls), it looks like when
files are changed, that there isn't even CompilerHost based incremental
support. (Please correct me if I'm wrong). So users of a very scaled
angular application would see slow build times, (even in watch mode).

Almost like trying to compile the angular/angular tsc src every time a
file changes. 👎 Is this the path we want to take?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8759 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAC5IxHKlt6qWieQuAK2t_ziyTlvrsklks5qLdwwgaJpZM4IjoJn
.

@MattJustMatt
Copy link

Wanted to check up on this. Currently blocking our prototyping for our migration for ng2, so anxious to see progress here!

@IAMtheIAM
Copy link

We need offline compilation with webpack. My angular2 bundle is getting too large, almost 8MB for all the polyfills, angular2 stuff, compiler stuff. Of that 8MB, only 600kb is my actually "app" code. Offline compilation would dramatically shrink my bundle size down, and make the app load way faster.

@TheLarkInn
Copy link
Member Author

TheLarkInn commented Aug 29, 2016

It's important to note it's possible by webpack 'ing the result of AoT compiler, the goal of this issue is to provide a direct webpack experience without having extra files in source, etc. The right webpack experience

@kylecordes
Copy link

Actually there has been discussion elsewhere of the affect of precompilation on overall deployed code size. Summary:

  • Small application, much smaller with precompilation, and faster to start.
  • Medium application, about the same size with precompilation, but faster to start.
  • Large application, much larger with precompilation, because the compiler output is larger than the template "source", and with enough templates this swamps the size of the JIT compiler download. precompilation will still yield a faster startup though, if you use lazy loading to avoiding loading all the code at once.

@TheLarkInn
Copy link
Member Author

TheLarkInn commented Aug 29, 2016

Could you link that here in your comment @kylecordes. Just for reference. And also, are you implying that webpack as a direct integration can help mitigate this?

@kylecordes
Copy link

@TheLarkInn It is discussed in #10812.

@tbosch
Copy link
Contributor

tbosch commented Sep 1, 2016

@robwormald Could you add a link of your integration of AoT with webpack here?

@TheLarkInn
Copy link
Member Author

@tbosch I took a look at it last night. I'd like to leave this issue open for now until we get it finalized.

@waeljammal
Copy link

+1 to this, our project is large and we had to go back to not using ngc during dev mode but now we waste a ton of time running builds to check if ngc still works. We should be able to just use it in both dev mode and build mode.

@IgorMinar IgorMinar added the area: core Issues related to the framework runtime label Oct 4, 2016
@vicb vicb added the feature Issue that requests a new feature label Oct 6, 2016
@DzmitryShylovich
Copy link
Contributor

@alexeagle webpack plugin is already available so I believe it can be closed

@TheLarkInn
Copy link
Member Author

The underlying issue with incremental compilation and diagnostics rooted in TS is not. But the AoT plugin does exist now. Having incremental compilation would severely enhance the AoT story but as of now there is something that works

@alexeagle
Copy link
Contributor

@tbosch is considering a true watch mode for ngc. I agree that the OP is solved as stated.

@MartinMa
Copy link
Contributor

@DzmitryShylovich What is the name of the webpack plugin you are referring to?

@DzmitryShylovich
Copy link
Contributor

@MartinMa https://www.npmjs.com/package/@ngtools/webpack

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime feature Issue that requests a new feature
Projects
None yet
Development

No branches or pull requests