feat(builder): broccoli bump to 1.0 by jkuri · Pull Request #299 · angular/angular-cli

jkuri · 2016-03-12T23:55:43Z

I rewritten TypeScript Broccoli Plugin to be compatible with Broccoli 1.0.
According to my tests builds are now ~50% faster.
I also added SASS and LESS plugins which works right off (SASS needs gem install sass) and they work with encapsulation.

filipesilva · 2016-03-13T22:19:49Z

I was under the impression that these files were compiled versions of the ones present in https://github.com/angular/angular/tree/master/tools/broccoli .

If that is so, then we shouldn't alter them ourselves. @hansl, do you have some more context on this?

jkuri · 2016-03-13T22:30:51Z

They are in the current master. I made some investigation on this as I did have feeling that this could work faster than 114ms per changed single file. I found out that some plugins (really complex ones) are not needed in 1.0 version of broccoli anymore (in my opinion) and these are the ones that are slowing down the compile/build time.
With this I got approximately 62ms build time per changed file, sort of like plain tsc.
I will not be offended if this is not going to be merged although I spend some time with this, just wanted to show what can be done with more simple approach comparing changed files. It isn't the more complex way always the best way, I think.
Did you try using this Filipe?

filipesilva · 2016-03-13T22:37:52Z

I did not try using it, no.

I was just going through the files on a top level review and remembered that we had problems with past PRs that changed them because they came from another repo, so I thought I should mention it. See #206 (comment)

I usually steer clear of those, because personally I do not understand them very well.

jkuri · 2016-03-13T22:45:17Z

I actually rewrite this specifically for our needs and did use some code from current one.
Lets see what Hans and Igor will say about this. In the meantime (when you will have time) please try this.. any feedback is welcome.

filipesilva · 2016-03-16T13:01:46Z

@jkuri had me in a videocall and stuff looked nice!

For me the most relevant bits were:

docs for the old brocolli version are out of date/unexisting
optional sass/less support without anything else done

Tbh if we can have sass/less without the user having to do anything (like ng install sass) and without overheads, I don't see why have them jump through hoops.

Still worried about desync'ing these files from the main angular repo. Also if we just abandon that build step, perhaps it's better to move to a different build system altogether. Dunno.

@hansl needs your input on this

jkuri · 2016-03-16T13:29:09Z

@hansl I would appreciate your opinion on this, when you will have time please ping me on slack.

zackarychapple · 2016-03-16T15:14:51Z

I want to echo @filipesilva on the worries as well as the need for a different build system. This discussion around builds and feature enhancements. Having a build system, particularly one that is extensible is going to be critical to the CLI.

jkuri · 2016-03-16T15:34:35Z

Maybe, but any "node package" aka compiler/preprocessor can be reworked as a broccoli plugin so I don't see a blocker here. I like the performance and I will advocate broccoli here as I think that is not possible to achieve better build times with anything else, at most equal. So, my vote goes to broccoli.

zackarychapple · 2016-03-16T17:00:41Z

So maybe we need a Broccoli equivalent of DefinitelyTyped where we wrap packages we want to use or others want to use in a Broccoli friendly way.

jkuri · 2016-03-16T17:06:01Z

You mean 3rd party packages? I am working on that right now with ng install command.
Otherwise, we'll move to webpack after the ng-conf if I remember correctly.

filipesilva · 2016-03-16T17:12:20Z

We definitely don't want to go down the angular-* approach to packages though. That's a problem with Angular 1 at the moment that we've talked about avoiding.

hansl · 2016-03-16T18:46:55Z

+
+ let trees = [vendorNpmTree, assetTree, tsNodeTree, jsTree, this.index()];
+
+ if (require.resolve('node-sass') && sh.which('sass')) {


So you're hard coding sass/less? What if the user wants to use Compass?

hansl · 2016-03-16T18:47:46Z

This PR is crazy; please reduce it to moving Broccoli to version 1, and remove the rest. We already want to cover SASS/LESS with ng install, there's no need for this.

jkuri · 2016-03-16T18:52:36Z

crazy :-) ok, will do.

hansl · 2016-03-16T20:46:16Z

+const Funnel = require('broccoli-funnel');
+const mergeTrees = require('broccoli-merge-trees');
+const ts = require('./broccoli-typescript');
+const sass = require('./broccoli-sass');


Requires not needed anymore.

hansl · 2016-03-16T20:46:39Z

+
+ const assetTree = new Funnel(src, {
+ include: ['**/*.*'],
+ exclude: ['**/*.ts', '**/*.js', '**/*.scss', '**/*.sass', '**/*.less'],


For now, just exclude ts, js and css.

hansl · 2016-03-17T16:45:52Z

You need to fix references to the broccoli stuff from the scaffold.

jkuri · 2016-03-17T16:57:58Z

Thanks, I would forgot this.

On Thursday, 17 March 2016, Hans notifications@github.com wrote:

You need to fix references to the broccoli stuff from the scaffold.
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#299 (comment)

hansl · 2016-03-17T19:17:08Z

 "lodash": "^4.6.1",
 "resolve": "^1.1.7",
 "shelljs": "^0.6.0",
+ "multidep": "^2.0.0",


not needed.

This is used by broccoli 1.0 for installing additional plugins. Does not work without that for me.

hansl · 2016-03-21T02:41:52Z

Unfortunately closing this in favor of #332. It's just really inconvenient for me to push to a branch forked on jkuri's account, so instead I'll just make a new PR.

angular-automatic-lock-bot · 2019-09-09T22:41:57Z

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.}

jkuri added type: enhancement labels Mar 12, 2016

jkuri force-pushed the broccoli-1.0beta branch 8 times, most recently from bd03f24 to d9646feCompare March 14, 2016 05:00

hansl reviewed Mar 16, 2016
View reviewed changes

jkuri force-pushed the broccoli-1.0beta branch from d9646fe to 450b5cfCompare March 16, 2016 19:51

jkuri changed the title ~~feat(builder): broccoli bump to 1.0 + LESS & SASS Plugins~~feat(builder): broccoli bump to 1.0Mar 16, 2016

feat(builder): broccoli bump to 1.0
c0fc136

jkuri force-pushed the broccoli-1.0beta branch from 450b5cf to c0fc136Compare March 16, 2016 20:01

hansl reviewed Mar 16, 2016
View reviewed changes

jkuri mentioned this pull request Mar 17, 2016
fix(serve): rebuilds after new typings are installed #295
Closed

hansl reviewed Mar 17, 2016
View reviewed changes

jkuri assigned hansl Mar 18, 2016

hansl closed this Mar 21, 2016

hansl mentioned this pull request Mar 21, 2016
feat(builder): broccoli bump to 1.0 #332
Merged

jkuri deleted the broccoli-1.0beta branch April 3, 2016 00:06

angular-automatic-lock-botBot locked and limited conversation to collaborators Sep 9, 2019


		let trees = [vendorNpmTree, assetTree, tsNodeTree, jsTree, this.index()];

		if (require.resolve('node-sass') && sh.which('sass')) {

Uh oh!

Conversation

jkuri commented Mar 12, 2016

Uh oh!

filipesilva commented Mar 13, 2016

Uh oh!

jkuri commented Mar 13, 2016

Uh oh!

filipesilva commented Mar 13, 2016

Uh oh!

jkuri commented Mar 13, 2016

Uh oh!

filipesilva commented Mar 16, 2016

Uh oh!

jkuri commented Mar 16, 2016

Uh oh!

zackarychapple commented Mar 16, 2016

Uh oh!

jkuri commented Mar 16, 2016

Uh oh!

zackarychapple commented Mar 16, 2016

Uh oh!

jkuri commented Mar 16, 2016

Uh oh!

filipesilva commented Mar 16, 2016

Uh oh!

hanslMar 16, 2016

Choose a reason for hiding this comment

Uh oh!

hansl commented Mar 16, 2016

Uh oh!

jkuri commented Mar 16, 2016

Uh oh!

hanslMar 16, 2016

Choose a reason for hiding this comment

Uh oh!

hanslMar 16, 2016

Choose a reason for hiding this comment

Uh oh!

hansl commented Mar 17, 2016

Uh oh!

jkuri commented Mar 17, 2016

Uh oh!

hanslMar 17, 2016

Choose a reason for hiding this comment

Uh oh!

jkuriMar 18, 2016

Choose a reason for hiding this comment

Uh oh!

hansl commented Mar 21, 2016

Uh oh!

angular-automatic-lock-botBot commented Sep 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants