fix(bazel): add zone-testing bundle for bazel build #982
Conversation
alexeagle
left a comment
alexeagle
left a comment
There was a problem hiding this comment.
This is great, it would make my example angular-bazel app a lot simpler, fixing one of these three blocks
https://github.com/alexeagle/angular-bazel-example/blob/master/BUILD.bazel
the other two I think I can fix by making angular a source dependency.
/cc @mhevery
gulpfile.js
Outdated
| return generateScript('./lib/testing/zone-testing.ts', 'zone-testing.js', false, cb); | ||
| }); | ||
|
|
||
| gulp.task('build/zone-testing.min.js', ['compile-esm'], function(cb) { |
There was a problem hiding this comment.
IIUC there is no benefit to using a minified script in tests, the only possible thing someone could do is make their debugging harder. They don't care about how many bytes are loaded in Karma. what do you think?
There was a problem hiding this comment.
yes, I agree, I will remove them.
gulpfile.js
Outdated
| }); | ||
|
|
||
| gulp.task('build/zone-testing-bundle.min.js', ['compile-esm'], function(cb) { | ||
| return generateScript('./lib/browser/rollup-test-main.ts', 'zone-testing-bundle.min.js', true, cb); |
There was a problem hiding this comment.
same as comment below about minified test bundle
There was a problem hiding this comment.
yes, you are right, we don't need minified bundle.
| 'build/zone.js', | ||
| 'build/zone.js.d.ts', | ||
| 'build/zone.min.js', | ||
| 'build/zone-testing.js', |
There was a problem hiding this comment.
the scripts needed are different depending if you are testing in node or in a browser.
For node, you need to start with zone-node.js, like Misko did in this PR:
https://github.com/angular/angular/pull/21053/files#diff-8ac936419da1e3d027833281525dba0e
Maybe we should also publish zone-node-testing.js?
There was a problem hiding this comment.
got it, I will make zone-node-testing-bundle
| import '../zone-spec/long-stack-trace'; | ||
| import '../zone-spec/proxy'; | ||
| import '../zone-spec/sync-test'; | ||
| import '../jasmine/jasmine'; |
There was a problem hiding this comment.
this makes an assumption about zone and jasmine being installed as siblings. Why not just import jasmine?
There was a problem hiding this comment.
Sorry I am not sure I understand your suggestion, I just import ../jasmine/jasmine which include zone patch for jasmine method.
There was a problem hiding this comment.
Oh, I see, I thought this was importing jasmine library itself, but it's https://github.com/angular/zone.js/blob/master/lib/jasmine/jasmine.ts
The user does need to ensure that jasmine itself is loaded before the zone-testing bundle
There was a problem hiding this comment.
yes, if jasmine is not loaded, here will throw error Missing: jasmine.js
|
@alexeagle , thank you for review, I have updated the PR. please review.
|
|
LGTM, I think we should probably let Misko look before merging? /cc @hansl about improving the UX for CLI users |
|
@alexeagle , got it. thank you! @mhevery , please review, and about |
4 participants
fix #404.
add
zone-testing-bundle.jsbundle andzone-testing.js.bazelbuild for Importing angular libraries and bootstrap is messy angular#21048 (comment)in this bundle, following modules are loaded.
@alexeagle , please review.
in this file, only test related files are loaded (in correct order).
so in
angular/cligeneratedsrc/test.ts, user don't need to load individual zone test files themselves, they can just load