build: add secondary entry points for tokens by Toxicable · Pull Request #922 · angular/universal

Toxicable · 2018-03-10T00:05:54Z

upgrades angular to v6-rc.1
uses bazel to do secondary entrypoints for tokens

Toxicable · 2018-03-11T09:13:24Z

Ran into some issue with not being able to use out reexported ng_module
Working around it by using the one exported from angular instead.
But otherwise I think this is good for review, havn't tested if this actually solve the issue yet though, havn't had the time

Toxicable · 2018-03-11T17:17:48Z

Looks like CI is getting some very unhelpful error.

Toxicable · 2018-03-12T23:38:25Z


 import { FileLoader } from './file-loader';
-import { REQUEST, RESPONSE } from './tokens';
+//  import { REQUEST, RESPONSE } from './tokens/index';


Toxicable · 2018-03-12T23:38:33Z

 * Use of this source code is governed by an MIT-style license that can be
 * found in the LICENSE file at https://angular.io/license
 */
+


Toxicable · 2018-03-12T23:41:18Z

 * Use of this source code is governed by an MIT-style license that can be
 * found in the LICENSE file at https://angular.io/license
 */
+


Toxicable · 2018-03-12T23:41:53Z

+load(
+    "//tools:defaults.bzl",
+    "ts_library",
+    # "ng_module",


add comment explaining this

Toxicable · 2018-03-12T23:42:18Z

 * found in the LICENSE file at https://angular.io/license
 */
-import { Request, Response } from 'hapi';
+


remove this

axnsan12 · 2018-03-13T09:11:47Z

Hello,

I see that this is marked as a breaking change and milestoned for 6.0.
I think this is wrong - at least for express-engine, because I had sucesfully been using import {REQUEST} from '@nguniversal/express-engine/tokens'; in "@nguniversal/express-engine": "^5.0.0-beta.5".

However after "upgrading" to "@nguniversal/express-engine": "^5.0.0-beta.6",, I was forced to change to import {REQUEST} from '@nguniversal/express-engine'; which increased my bundle size by the mentioned 600kb.

I don't think you should force us to wait for 6.0 for a fix to a regression introduced in a minor version.

Just my 2 cents, and thank you for your good work on this project!

Toxicable · 2018-03-13T16:49:22Z

@axnsan12 sorry about the confusion, this is definitely going into 5.x, notice the PR Target 5.x & master.
Working on it as much as i can, should hopefully be released by end of week.

Toxicable · 2018-03-17T21:51:04Z

Just a small update here.
I've made this fix with another PR here: #930
which has been merged, just awaiting a release.

Toxicable · 2018-03-22T08:41:43Z

This is almost ready to go, just 1 small issue which is possibly due to jasmine_node_test

==================== Test output for //modules/common:unit_test:
failed to load main  ReferenceError: HTTP_INTERCEPTORS is not defined
    at /home/fabian/.cache/bazel/_bazel_fabian/5bf8e1a5e0d57cedec3042cf2af51481/execroot/nguniversal/bazel-out/k8-fastbuild/bin/modules/common/src/transfer_http.ngf
actory.js:42:32

the generated code is:

    var HTTP_INTERCEPTORS_20 = HTTP_INTERCEPTORS;
    exports.HTTP_INTERCEPTORS_20 = HTTP_INTERCEPTORS_20;

Looking for sign off from @alexeagle

disabling this test for now

alexeagle · 2018-04-02T16:06:07Z

 exports_files(["tsconfig.json"])

-# Provides the npm-installed dependencies to Bazel actions.
 filegroup(


there is now an (undocumented) node_modules_filegroup that you should probably use instead, just for readability: https://github.com/bazelbuild/rules_nodejs/blob/master/defs.bzl#L41

alexeagle · 2018-04-02T16:06:59Z

@@ -0,0 +1,3 @@
+{
+  "name": "@nguniversal/hapi-engine/tokens"


hmm, you shouldn't need to add extra package.json files - ng_package should generate these for secondary entry points as needed

vikerman · 2018-04-04T12:08:33Z

+def ts_test_library(node_modules=None, **kwargs):
+    ts_library(testonly=1, **kwargs)
+
+def ts_library(tsconfig = None, node_modules = None, **kwargs):


Why are ts_library, ng_module and ng_package redefined here? I would rather prefer us specifying this everywhere by hand,

Inside Google we have learnt never to use macros except for final leaf targets and when completely unavoidable.

While I agree it's not the best idea, it's so we don't have to redefine those variables and can keep them organised in one place since there is no alternative right now.
This is the same as Angular core https://github.com/angular/angular/blob/master/tools/defaults.bzl
Once we have a better way of specifying defaults then we wont need this.

Alright - Spoke with Alex and this is a temporary work around needed. Let's add a TODO comment that this should be revisited at some point.

vikerman · 2018-04-04T12:09:29Z

      "@nguniversal/hapi-engine/tokens": "nguniversal.hapiEngine.tokens",
      "rxjs": "Rx",
-      "rxjs/operators": "Rx.operators"
+      "rxjs/operators": "Rx.operators",


I don't completely understand what the GLOBALS is doing. But I'm scared seeing the symbol Rx here. Rx.ts in rxjs pulls in the entire library. Are we sure that's not happening here?

This is ensuring that the entire lib is not being pulled into the umd bundle. The symbol on the right is what it will be named on the global object when passed in.

angular-automatic-lock-bot · 2019-09-06T00:32:30Z

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

Toxicable requested a review from vikerman as a code owner March 10, 2018 00:05

googlebot added the cla: yes label Mar 10, 2018

CaerusKaru added state: WIP area: build & ci labels Mar 10, 2018

CaerusKaru added this to the 6.0 milestone Mar 10, 2018

Toxicable added the target: major This PR is targeted for the next major release label Mar 10, 2018

Toxicable requested review from CaerusKaru and MarkPieszak as code owners March 11, 2018 08:05

Toxicable force-pushed the fabian/secondary-entry branch 2 times, most recently from c26a873 to e8a819e Compare March 11, 2018 09:12

Toxicable force-pushed the fabian/secondary-entry branch from e8a819e to 47ec44f Compare March 11, 2018 09:13

Toxicable added action: review and removed state: WIP labels Mar 11, 2018

Toxicable force-pushed the fabian/secondary-entry branch from 47ec44f to 78f18f5 Compare March 11, 2018 09:20

Toxicable force-pushed the fabian/secondary-entry branch from 02742be to 1f570dd Compare March 12, 2018 07:09

Toxicable commented Mar 12, 2018

View reviewed changes

Toxicable force-pushed the fabian/secondary-entry branch 6 times, most recently from c7e5bc1 to 334443f Compare March 18, 2018 05:08

Toxicable added target: minor target: minor This PR is targeted for the next minor release and removed target: major This PR is targeted for the next major release labels Mar 23, 2018

Toxicable added cla: yes and removed cla: no labels Mar 24, 2018

Toxicable added state: WIP and removed action: review labels Mar 31, 2018

Toxicable force-pushed the fabian/secondary-entry branch 5 times, most recently from 882b3a0 to c09f185 Compare March 31, 2018 06:55

Toxicable added action: review and removed state: WIP labels Mar 31, 2018

chore: upgrade to v6

5f26cd4

Toxicable force-pushed the fabian/secondary-entry branch from 4f427ab to 47daaa7 Compare April 2, 2018 04:11

CaerusKaru mentioned this pull request Apr 2, 2018

build: add Bazel rules for secondary entrypoints #950

Closed

alexeagle approved these changes Apr 2, 2018

View reviewed changes

Toxicable force-pushed the fabian/secondary-entry branch 4 times, most recently from 64ac52f to 0948ce1 Compare April 3, 2018 08:31

build: use bazel for secondary entry point for tokens

8d7beef

Toxicable force-pushed the fabian/secondary-entry branch from 0948ce1 to 8d7beef Compare April 3, 2018 09:53

vikerman suggested changes Apr 4, 2018

View reviewed changes

build: add tslib to globals

bb179ba

vikerman approved these changes Apr 4, 2018

View reviewed changes

Fabian Wiles added 2 commits April 5, 2018 08:42

build: add TODO for macros

6f23c51

build: fix note about default macros

c7ac93c

Toxicable merged commit 01b3c50 into master Apr 4, 2018

Toxicable deleted the fabian/secondary-entry branch April 4, 2018 20:57

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 6, 2019

Conversation

Toxicable commented Mar 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Toxicable commented Mar 11, 2018

Uh oh!

Toxicable commented Mar 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axnsan12 commented Mar 13, 2018

Uh oh!

Toxicable commented Mar 13, 2018

Uh oh!

Toxicable commented Mar 17, 2018

Uh oh!

Toxicable commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot Bot commented Sep 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Toxicable commented Mar 10, 2018 •

edited

Loading

Toxicable commented Mar 22, 2018 •

edited

Loading