Skip to content

feat(builder): broccoli bump to 1.0 #332

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 23, 2016
Merged

feat(builder): broccoli bump to 1.0 #332

merged 2 commits into from
Mar 23, 2016

Conversation

hansl
Copy link
Contributor

@hansl hansl commented Mar 21, 2016

This is the right PR for the broccoli upgrade.

Original was #299.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@jkuri
Copy link
Contributor

jkuri commented Mar 21, 2016

Cool! I will close mine PR but leave local branch if you find anything useful.

@hansl hansl mentioned this pull request Mar 21, 2016
Closed
@hansl hansl force-pushed the broccoli1 branch 6 times, most recently from 85414a2 to 3e4a470 Compare March 22, 2016 05:25
@filipesilva
Copy link
Contributor

I went over the changes and it lgtm. Also checked the CI logs and couldn't find any weird stuff.

@filipesilva
Copy link
Contributor

Setting "compileOnSave": false, on tsconfig.json makes every save break on my IDE (Atom).

I thought it might be a sign that there's something wrong with manual compilation. Compiling with tsc -p works, but compiling directly from the src dir does not.

filipe.silva@DESKTOP-7ND6T3R MINGW64 /E/dev/ideablade/testing/testproj2 (master)
$ tsc -p src/

filipe.silva@DESKTOP-7ND6T3R MINGW64 /E/dev/ideablade/testing/testproj2 (master)
$ cd src/

filipe.silva@DESKTOP-7ND6T3R MINGW64 /E/dev/ideablade/testing/testproj2/src (master)
$ tsc
app.ts(1,25): error TS2307: Cannot find module 'angular2/platform/browser'.
app/testproj2.ts(1,25): error TS2307: Cannot find module 'angular2/core'.
app/testproj2.ts(2,64): error TS2307: Cannot find module 'angular2/router'.

@filipesilva
Copy link
Contributor

Running ng build will generate .js and .js.map files inside src/app. @jkuri and I were able to reproduce this.

jkuri
jkuri reviewed Mar 22, 2016
View reviewed changes
lib/broccoli/broccoli-typescript.js
* re-emitted. This primarily affects code that uses `const enum`s, because changing the enum value
* requires global emit, which can affect many files.
*/
class DiffingTSCompiler extends Plugin {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 nice

@hansl
Copy link
Contributor Author

hansl commented Mar 22, 2016

@filipesilva I fixed the bug where js and js.map were generated in the source directory. Everything should be fine now.

@filipesilva
Copy link
Contributor

Tested and can confirm it's fixed.

jkuri
jkuri reviewed Mar 22, 2016
View reviewed changes
lib/broccoli/angular2-app.js
}));
// Add all spec files to files. We need this because spec files are their own entry
// point.
fs.readdirSync(sourceDir).forEach(function addPathRecursive(name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

jkuri
jkuri reviewed Mar 22, 2016
View reviewed changes
lib/broccoli/angular2-app.js
@@ -113,10 +111,10 @@ Angular2App.prototype.toTree = function() {
jsTree,
this.index(),
vendorNpmTree,
thirdPartyJs
thirdPartyJs,
Copy link
Contributor

Choose a reason for hiding this comment

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

can you remove this please, we are not using it anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@hansl hansl force-pushed the broccoli1 branch 3 times, most recently from 2925f7a to 9cc3633 Compare March 22, 2016 15:46
@hansl
Copy link
Contributor Author

hansl commented Mar 22, 2016

Also added a test to make sure ng build doesn't change anything in git.

@jkuri
Copy link
Contributor

jkuri commented Mar 22, 2016

I still need to bug a bit, unfortunately there is still issue when some .ts file changes;
file changed app/proj1.ts ENOENT: no such file or directory, scandir '/Users/jan/Desktop/angular-cli/proj1/tmp/funnel-input_base_path-tOrNtqOJ.tmp/0/src/'
After this will be solved it is ready for merge, I will help you check it.

Btw, you increased performance by 1/3 👍

@jkuri
Copy link
Contributor

jkuri commented Mar 22, 2016

I cannot comment on the lines that are not being changed so I will write fixes here;

  • in broccoli-typescript.js:115 add o.name = path.join(_this.outputPath, o.name);
  • in service-worker-manifest.js:40 replace with var manifestPath = path.join(this.cachePath, MANIFEST_FILE)

Please let me know when you will be done so I can check it.

@hansl
Copy link
Contributor Author

hansl commented Mar 22, 2016

Fixed what I think were all bugs. Please take another look.

@alxhub: could you take a look at the SWManifest? I stopped reading the manifest file and instead keep the hash in memory.

@jkuri
Copy link
Contributor

jkuri commented Mar 22, 2016

LGTM

@hansl hansl merged commit 1819afe into master Mar 23, 2016
@hansl hansl deleted the broccoli1 branch March 23, 2016 01:16
@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 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hansl @googlebot @jkuri @filipesilva