Skip to content
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

refactor(@angular/ssr): bundle Critters #28228

Merged
merged 4 commits into from
Aug 24, 2024
Merged

refactor(@angular/ssr): bundle Critters #28228

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0b7679d
refactor(@angular/ssr): bundle Critters
alan-agius4 Aug 22, 2024
99c9e7a
refactor: add TypeScript declaration merging for Critters
alan-agius4 Aug 23, 2024
f1d3fe0
build: add function to validate and update 3rd party license files
alan-agius4 Aug 23, 2024
1c70f47
build: use Bazel `diff_test` to compare file differences
alan-agius4 Aug 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@
"@rollup/plugin-node-resolve": "^13.0.5",
"@types/babel__core": "7.20.5",
"@types/browser-sync": "^2.27.0",
"@types/diff": "^5.2.1",
"@types/express": "^4.16.0",
"@types/http-proxy": "^1.17.4",
"@types/ini": "^4.0.0",
Expand Down Expand Up @@ -132,7 +131,6 @@
"critters": "0.0.24",
"css-loader": "7.1.2",
"debug": "^4.1.1",
"diff": "^5.2.0",
"esbuild": "0.23.1",
"esbuild-wasm": "0.23.1",
"eslint": "8.57.0",
Expand Down
1 change: 0 additions & 1 deletion packages/angular/ssr/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
"@angular/platform-server": "19.0.0-next.1",
"@angular/router": "19.0.0-next.1",
"@bazel/runfiles": "^5.8.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question: Would @bazel/runfiles make more sense in the root package.json? I'm kinda surprised it's not there already.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is there: https://github.com/angular/angular-cli/pull/28228/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R86

"diff": "^5.2.0",
"zone.js": "^0.15.0"
},
"schematics": "./schematics/collection.json",
Expand Down
47 changes: 35 additions & 12 deletions packages/angular/ssr/test/npm_package/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_binary")
load("@bazel_skylib//rules:diff_test.bzl", "diff_test")
load("@bazel_skylib//rules:write_file.bzl", "write_file")
load("@npm//@bazel/jasmine:index.bzl", "jasmine_node_test")
load("//tools:defaults.bzl", "ts_library")

Expand All @@ -8,28 +9,50 @@ ts_library(
srcs = glob(["**/*.ts"]),
deps = [
"@npm//@bazel/runfiles",
"@npm//@types/diff",
],
)

jasmine_node_test(
name = "test",
srcs = [":unit_test_lib"],
data = [
"THIRD_PARTY_LICENSES.txt.golden",
"//packages/angular/ssr:npm_package",
"@npm//diff",
],
)

nodejs_binary(
name = "test.accept",
testonly = True,
data = [
"THIRD_PARTY_LICENSES.txt.golden",
":unit_test_lib",
genrule(
name = "critters_license_file",
srcs = [
"//packages/angular/ssr:npm_package",
"@npm//diff",
],
entry_point = ":update-package-golden.ts",
outs = [
"THIRD_PARTY_LICENSES.txt",
],
cmd = """
cp $(location //packages/angular/ssr:npm_package)/third_party/critters/THIRD_PARTY_LICENSES.txt $(location :THIRD_PARTY_LICENSES.txt)
""",
)

diff_test(
name = "critters_license_test",
failure_message = """

To accept the new golden file, execute:
yarn bazel run //packages/angular/ssr/test/npm_package:critters_license_test.accept
""",
file1 = ":THIRD_PARTY_LICENSES.txt.golden",
file2 = ":critters_license_file",
)

write_file(
name = "critters_license_test.accept",
out = "critters_license_file_accept.sh",
content =
[
"#!/usr/bin/env bash",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Neat trick with write_file, didn't know it could create a binary like that.

One limitation here is that this will only work for Unix developers, not Windows devs. It's probably not a huge deal, especially since anyone could manually copy over the text, even if that is a bit annoying.

Maybe once we get on @aspect_rules_js it might be worth making a more reusable and cross-platform golden_test macro. I thought @aspect_bazel_lib had something here, but it seems like their diff_test isn't much smarter than Skylib's.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could create a macro that creates a CMD instead a bash for Windows.

But as mentioned there are workaround, and likely the update of the golden will be done by one of team which at the moment use Linux or Mac.

"cd ${BUILD_WORKSPACE_DIRECTORY}",
"yarn bazel build //packages/angular/ssr:npm_package",
"cp -fv dist/bin/packages/angular/ssr/npm_package/third_party/critters/THIRD_PARTY_LICENSES.txt packages/angular/ssr/test/npm_package/THIRD_PARTY_LICENSES.txt.golden",
],
is_executable = True,
)
60 changes: 23 additions & 37 deletions packages/angular/ssr/test/npm_package/package_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,32 @@
* found in the LICENSE file at https://angular.dev/license
*/

import { createPatch } from 'diff';
import { runfiles } from '@bazel/runfiles';
import { existsSync } from 'node:fs';
import { readFile } from 'node:fs/promises';
import { join } from 'node:path';
import {
import { dirname, join } from 'node:path';

/**
* Resolve paths for the Critters license file and the golden reference file.
*/
const ANGULAR_SSR_PACKAGE_PATH = dirname(
runfiles.resolve('angular_cli/packages/angular/ssr/npm_package/package.json'),
);

/**
* Path to the actual license file for the Critters library.
* This file is located in the `third_party/critters` directory of the Angular CLI npm package.
*/
const CRITTERS_ACTUAL_LICENSE_FILE_PATH = join(
ANGULAR_SSR_PACKAGE_PATH,
CRITTERS_ACTUAL_LICENSE_FILE_PATH,
CRITTERS_GOLDEN_LICENSE_FILE_PATH,
} from './utils';
'third_party/critters/THIRD_PARTY_LICENSES.txt',
);

/**
* Path to the golden reference license file for the Critters library.
* This file is used as a reference for comparison and is located in the same directory as this script.
*/
const CRITTERS_GOLDEN_LICENSE_FILE_PATH = join(__dirname, 'THIRD_PARTY_LICENSES.txt.golden');

describe('NPM Package Tests', () => {
it('should not include the contents of third_party/critters/index.js in the FESM bundle', async () => {
Expand All @@ -27,36 +44,5 @@ describe('NPM Package Tests', () => {
it('should exist', () => {
expect(existsSync(CRITTERS_ACTUAL_LICENSE_FILE_PATH)).toBe(true);
});

it('should match the expected golden file', async () => {
const [expectedContent, actualContent] = await Promise.all([
readFile(CRITTERS_GOLDEN_LICENSE_FILE_PATH, 'utf-8'),
readFile(CRITTERS_ACTUAL_LICENSE_FILE_PATH, 'utf-8'),
]);

if (expectedContent.trim() === actualContent.trim()) {
return;
}

const patch = createPatch(
CRITTERS_GOLDEN_LICENSE_FILE_PATH,
expectedContent,
actualContent,
'Golden License File',
'Current License File',
{ context: 5 },
);

const errorMessage = `The content of the actual license file differs from the expected golden reference.
Diff:
${patch}
To accept the new golden file, execute:
yarn bazel run ${process.env['BAZEL_TARGET']}.accept
`;

const error = new Error(errorMessage);
error.stack = error.stack?.replace(` Diff:\n ${patch}`, '');
throw error;
});
});
});
15 changes: 0 additions & 15 deletions packages/angular/ssr/test/npm_package/update-package-golden.ts
View file
Open in desktop

This file was deleted.

32 changes: 0 additions & 32 deletions packages/angular/ssr/test/npm_package/utils.ts
View file
Open in desktop

This file was deleted.

12 changes: 1 addition & 11 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,6 @@ __metadata:
"@rollup/plugin-node-resolve": "npm:^13.0.5"
"@types/babel__core": "npm:7.20.5"
"@types/browser-sync": "npm:^2.27.0"
"@types/diff": "npm:^5.2.1"
"@types/express": "npm:^4.16.0"
"@types/http-proxy": "npm:^1.17.4"
"@types/ini": "npm:^4.0.0"
Expand Down Expand Up @@ -708,7 +707,6 @@ __metadata:
critters: "npm:0.0.24"
css-loader: "npm:7.1.2"
debug: "npm:^4.1.1"
diff: "npm:^5.2.0"
esbuild: "npm:0.23.1"
esbuild-wasm: "npm:0.23.1"
eslint: "npm:8.57.0"
Expand Down Expand Up @@ -969,7 +967,6 @@ __metadata:
"@angular/platform-server": "npm:19.0.0-next.1"
"@angular/router": "npm:19.0.0-next.1"
"@bazel/runfiles": "npm:^5.8.1"
diff: "npm:^5.2.0"
tslib: "npm:^2.3.0"
zone.js: "npm:^0.15.0"
peerDependencies:
Expand Down Expand Up @@ -4871,13 +4868,6 @@ __metadata:
languageName: node
linkType: hard

"@types/diff@npm:^5.2.1":
version: 5.2.1
resolution: "@types/diff@npm:5.2.1"
checksum: 10c0/62dcab32197ac67f212939cdd79aa3953327a482bec55c6a38ad9de8a0662a9f920b59504609a322fc242593bd9afb3d2704702f4bc98087a13171234b952361
languageName: node
linkType: hard

"@types/estree@npm:*, @types/estree@npm:1.0.5, @types/estree@npm:^1.0.0, @types/estree@npm:^1.0.5":
version: 1.0.5
resolution: "@types/estree@npm:1.0.5"
Expand Down Expand Up @@ -8561,7 +8551,7 @@ __metadata:
languageName: node
linkType: hard

"diff@npm:^5.0.0, diff@npm:^5.1.0, diff@npm:^5.2.0":
"diff@npm:^5.0.0, diff@npm:^5.1.0":
version: 5.2.0
resolution: "diff@npm:5.2.0"
checksum: 10c0/aed0941f206fe261ecb258dc8d0ceea8abbde3ace5827518ff8d302f0fc9cc81ce116c4d8f379151171336caf0516b79e01abdc1ed1201b6440d895a66689eb4
Expand Down