ocket8888 commented on code in PR #7582:
URL: https://github.com/apache/trafficcontrol/pull/7582#discussion_r1235597264
##########
experimental/traffic-portal/server.ts:
##########
@@ -45,18 +90,67 @@ let config: ServerConfig;
export function app(serverConfig: ServerConfig): express.Express {
const server = express();
const indexHtml = join(serverConfig.browserFolder, "index.html");
- if(!existsSync(indexHtml)) {
+ if (!existsSync(indexHtml)) {
throw new Error(`Unable to start TP server, unable to find
browser index.html at: ${indexHtml}`);
}
// Our Universal express-engine (found @
https://github.com/angular/universal/tree/master/modules/express-engine)
server.engine("html", ngExpressEngine({
- bootstrap: AppServerModule,
+ bootstrap: AppServerModule
}));
server.set("view engine", "html");
server.set("views", serverConfig.browserFolder);
+ const allFiles = getFiles(serverConfig.browserFolder);
+ const compressedFiles = new Map(allFiles
+ .filter(file => file.match(/\.br|gz$/))
+ .map(file => [file, undefined]));
+ const foundFiles = new Map<string, StaticFile>(allFiles
+ .filter(file => file.match(/\.js|css|tff|svg$/))
+ .map(file => {
+ const staticFile = {
+ compressions: []
+ } as StaticFile;
Review Comment:
You shouldn't need `as StaticFile` here; the compiler shouldn't be
complaining, but if it is, you can just do `const staticFile: StaticFile = ...`.
##########
experimental/traffic-portal/server.ts:
##########
@@ -45,18 +90,67 @@ let config: ServerConfig;
export function app(serverConfig: ServerConfig): express.Express {
const server = express();
const indexHtml = join(serverConfig.browserFolder, "index.html");
- if(!existsSync(indexHtml)) {
+ if (!existsSync(indexHtml)) {
throw new Error(`Unable to start TP server, unable to find
browser index.html at: ${indexHtml}`);
}
// Our Universal express-engine (found @
https://github.com/angular/universal/tree/master/modules/express-engine)
server.engine("html", ngExpressEngine({
- bootstrap: AppServerModule,
+ bootstrap: AppServerModule
}));
server.set("view engine", "html");
server.set("views", serverConfig.browserFolder);
+ const allFiles = getFiles(serverConfig.browserFolder);
+ const compressedFiles = new Map(allFiles
+ .filter(file => file.match(/\.br|gz$/))
Review Comment:
This matches all files that contain a `.br` _anywhere_ **or** end with `gz`.
Some examples of matches:
- test.gz
- test.br
- test.tgz
- t.brap.zip
- foogz
- .br.rc
I think what you actually want is `/\.(br|gz)$/`.
##########
experimental/traffic-portal/server.ts:
##########
@@ -45,18 +90,67 @@ let config: ServerConfig;
export function app(serverConfig: ServerConfig): express.Express {
const server = express();
const indexHtml = join(serverConfig.browserFolder, "index.html");
- if(!existsSync(indexHtml)) {
+ if (!existsSync(indexHtml)) {
throw new Error(`Unable to start TP server, unable to find
browser index.html at: ${indexHtml}`);
}
// Our Universal express-engine (found @
https://github.com/angular/universal/tree/master/modules/express-engine)
server.engine("html", ngExpressEngine({
- bootstrap: AppServerModule,
+ bootstrap: AppServerModule
}));
server.set("view engine", "html");
server.set("views", serverConfig.browserFolder);
+ const allFiles = getFiles(serverConfig.browserFolder);
+ const compressedFiles = new Map(allFiles
+ .filter(file => file.match(/\.br|gz$/))
+ .map(file => [file, undefined]));
+ const foundFiles = new Map<string, StaticFile>(allFiles
+ .filter(file => file.match(/\.js|css|tff|svg$/))
+ .map(file => {
+ const staticFile = {
+ compressions: []
+ } as StaticFile;
+ if (compressedFiles.has(`${file}.${br.fileExt}`)) {
+ staticFile.compressions.push(br);
+ }
+ if (compressedFiles.has(`${file}.${gzip.fileExt}`)) {
+ staticFile.compressions.push(gzip);
+ }
+ return [file, staticFile];
+ }));
+
+ const typeMap = new Map([
+ ["js", "application/javascript"],
+ ["css", "text/css"],
+ ["ttf", "font/ttf"],
+ ["svg", "image/svg+xml"]
+ ]);
+ // Could just use express compression `server.use(compression())` but
that is calculated for each request
+ server.get("*.(js|css|ttf|svg)", function(req, res, next) {
+ const type = req.url.split(".").pop();
Review Comment:
This can cause problems with query strings, which I _think_ Angular uses for
cachebusting? Not sure. But it would for sure be safer to access
[`req.path`](https://expressjs.com/en/api.html#req.path) instead of `req.url`.
##########
experimental/traffic-portal/src/app/core/cache-groups/asns/detail/asn-detail.component.html:
##########
@@ -29,6 +29,11 @@
<mat-select name="cachegroup"
[(ngModel)]="asn.cachegroupId" required>
<mat-option *ngFor="let cachegroup of
cachegroups" [value]="cachegroup.id">{{cachegroup.name}}</mat-option>
</mat-select>
+ <mat-hint>
+ <a mat-icon-button
[disabled]="!asn.cachegroupId" class="small-icon-button" matTooltip="View Cache
Group Details" aria-label="View Cache Group Details" color="primary"
[routerLink]="'/core/cache-groups/' + asn.cachegroupId" target="_blank">
Review Comment:
You don't need to change this, but `aria-label` is unnecessary here, because
`matTooltip` will add an `aria-describedby` pointing to an element with the
given text.
##########
experimental/traffic-portal/src/app/core/cache-groups/regions/detail/region-detail.component.html:
##########
@@ -12,19 +12,27 @@
limitations under the License.
-->
-<mat-card appearance="outlined">
+<mat-card appearance="outlined" class="page-content single-column">
<tp-loading *ngIf="!region"></tp-loading>
<form ngNativeValidate (ngSubmit)="submit($event)" *ngIf="region">
- <mat-card-content>
+ <mat-card-header class="headers-container" *ngIf="!new">
+ <a mat-raised-button color="primary"
[routerLink]="'/core/phys-locs'" [queryParams]="{region: region.name}">View
Phys Locations</a>
Review Comment:
> View Phys<ins>ical</ins> Locations
##########
experimental/traffic-portal/src/app/core/deliveryservice/new-delivery-service/new-delivery-service.component.spec.ts:
##########
@@ -12,32 +12,9 @@
* limitations under the License.
*/
-import { type HarnessLoader, parallel } from "@angular/cdk/testing";
-import { TestbedHarnessEnvironment } from "@angular/cdk/testing/testbed";
-import { HttpClientModule } from "@angular/common/http";
-import { type ComponentFixture, TestBed } from "@angular/core/testing";
-import { FormsModule, ReactiveFormsModule } from "@angular/forms";
-import { MatButtonModule } from "@angular/material/button";
-import { MatRadioModule } from "@angular/material/radio";
-import { MatStepperModule } from "@angular/material/stepper";
-import { MatStepperHarness } from "@angular/material/stepper/testing";
-import { NoopAnimationsModule } from "@angular/platform-browser/animations";
-import { RouterTestingModule } from "@angular/router/testing";
-import { ReplaySubject } from "rxjs";
-import { Protocol } from "trafficops-types";
-
-import { APITestingModule } from "src/app/api/testing";
-import { CurrentUserService } from
"src/app/shared/current-user/current-user.service";
-import { NavigationService } from
"src/app/shared/navigation/navigation.service";
-import { TpHeaderComponent } from
"src/app/shared/navigation/tp-header/tp-header.component";
-
-import { NewDeliveryServiceComponent } from "./new-delivery-service.component";
-
describe("NewDeliveryServiceComponent", () => {
- let component: NewDeliveryServiceComponent;
- let fixture: ComponentFixture<NewDeliveryServiceComponent>;
- let loader: HarnessLoader;
+/*
Review Comment:
ummm.... why?
##########
experimental/traffic-portal/src/app/core/statuses/status-details/status-details.component.ts:
##########
@@ -46,21 +49,12 @@ export class StatusDetailsComponent {
name: new FormControl("", {nonNullable: true}),
});
- /**
- * Constructor.
- *
- * @param api The Servers API which is used to provide row data.
- * @param route A reference to the route of this view which is used to
get the 'id' query parameter of status.
- * @param router Angular router
- * @param dialog Dialog manager
- * @param fb Form builder
- * @param navSvc Manages the header
- */
constructor(
private readonly api: ServerService,
- private readonly route: ActivatedRoute,
+ public readonly route: ActivatedRoute,
Review Comment:
why does this need to be public?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
