From 10121b8d07f9d24c1afba956626e22dab095953a Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Fri, 19 Jan 2024 14:10:42 -0800 Subject: [PATCH] feat: add special handling to js_run_devserver for 1p deps to improve watch mode performance (#1411) --- e2e/webpack_devserver/.bazelignore | 3 +- e2e/webpack_devserver/BUILD.bazel | 2 + e2e/webpack_devserver/mylib/BUILD.bazel | 10 + e2e/webpack_devserver/mylib/index.js | 5 + e2e/webpack_devserver/mylib/package.json | 6 + e2e/webpack_devserver/package.json | 3 +- e2e/webpack_devserver/pnpm-lock.yaml | 43 +++- e2e/webpack_devserver/pnpm-workspace.yaml | 1 + e2e/webpack_devserver/serve_test.sh | 58 ++++- e2e/webpack_devserver/src/index.js | 3 +- e2e/webpack_devserver_esm/.bazelignore | 3 +- e2e/webpack_devserver_esm/BUILD.bazel | 1 + e2e/webpack_devserver_esm/mylib/BUILD.bazel | 10 + e2e/webpack_devserver_esm/mylib/index.js | 5 + e2e/webpack_devserver_esm/mylib/package.json | 6 + e2e/webpack_devserver_esm/package.json | 3 +- e2e/webpack_devserver_esm/pnpm-lock.yaml | 43 +++- e2e/webpack_devserver_esm/pnpm-workspace.yaml | 1 + e2e/webpack_devserver_esm/serve_test.sh | 54 ++++- e2e/webpack_devserver_esm/src/index.js | 3 +- js/private/js_run_devserver.bzl | 16 +- js/private/js_run_devserver.mjs | 207 ++++++++++++++---- .../js_run_devserver/{BUILD => BUILD.bazel} | 10 + .../js_run_devserver.spec.mjs | 80 +++++++ 24 files changed, 523 insertions(+), 53 deletions(-) create mode 100644 e2e/webpack_devserver/mylib/BUILD.bazel create mode 100644 e2e/webpack_devserver/mylib/index.js create mode 100644 e2e/webpack_devserver/mylib/package.json create mode 100644 e2e/webpack_devserver_esm/mylib/BUILD.bazel create mode 100644 e2e/webpack_devserver_esm/mylib/index.js create mode 100644 e2e/webpack_devserver_esm/mylib/package.json rename js/private/test/js_run_devserver/{BUILD => BUILD.bazel} (83%) create mode 100644 js/private/test/js_run_devserver/js_run_devserver.spec.mjs diff --git a/e2e/webpack_devserver/.bazelignore b/e2e/webpack_devserver/.bazelignore index b512c09d4..262c043e8 100644 --- a/e2e/webpack_devserver/.bazelignore +++ b/e2e/webpack_devserver/.bazelignore @@ -1 +1,2 @@ -node_modules \ No newline at end of file +node_modules +mylib/node_modules diff --git a/e2e/webpack_devserver/BUILD.bazel b/e2e/webpack_devserver/BUILD.bazel index d608a079a..950709f2b 100644 --- a/e2e/webpack_devserver/BUILD.bazel +++ b/e2e/webpack_devserver/BUILD.bazel @@ -28,6 +28,7 @@ js_run_devserver( "webpack.config.js", ":node_modules", ], + log_level = "debug", tool = "webpack_binary", ) @@ -45,5 +46,6 @@ js_run_devserver( "webpack.config.cjs", ":node_modules", ], + log_level = "debug", tool = "webpack_binary", ) diff --git a/e2e/webpack_devserver/mylib/BUILD.bazel b/e2e/webpack_devserver/mylib/BUILD.bazel new file mode 100644 index 000000000..428fe4988 --- /dev/null +++ b/e2e/webpack_devserver/mylib/BUILD.bazel @@ -0,0 +1,10 @@ +load("@aspect_rules_js//npm:defs.bzl", "npm_package") + +npm_package( + name = "mylib", + srcs = [ + "index.js", + "package.json", + ], + visibility = ["//visibility:public"], +) diff --git a/e2e/webpack_devserver/mylib/index.js b/e2e/webpack_devserver/mylib/index.js new file mode 100644 index 000000000..27630cec7 --- /dev/null +++ b/e2e/webpack_devserver/mylib/index.js @@ -0,0 +1,5 @@ +const packageJson = require('./package.json') +const chalk = require('chalk') +module.exports = { + name: () => chalk.blue(packageJson.name), +} diff --git a/e2e/webpack_devserver/mylib/package.json b/e2e/webpack_devserver/mylib/package.json new file mode 100644 index 000000000..b356402d8 --- /dev/null +++ b/e2e/webpack_devserver/mylib/package.json @@ -0,0 +1,6 @@ +{ + "name": "@mycorp/mylib", + "dependencies": { + "chalk": "^4" + } +} diff --git a/e2e/webpack_devserver/package.json b/e2e/webpack_devserver/package.json index 81f4664ff..8541cf53f 100644 --- a/e2e/webpack_devserver/package.json +++ b/e2e/webpack_devserver/package.json @@ -7,6 +7,7 @@ "@bazel/ibazel": "0.16.2" }, "dependencies": { - "lodash": "4.17.21" + "lodash": "4.17.21", + "@mycorp/mylib": "workspace:*" } } diff --git a/e2e/webpack_devserver/pnpm-lock.yaml b/e2e/webpack_devserver/pnpm-lock.yaml index b6f2931cc..bba49f6ef 100644 --- a/e2e/webpack_devserver/pnpm-lock.yaml +++ b/e2e/webpack_devserver/pnpm-lock.yaml @@ -8,6 +8,9 @@ importers: .: dependencies: + '@mycorp/mylib': + specifier: workspace:* + version: link:mylib lodash: specifier: 4.17.21 version: 4.17.21 @@ -28,6 +31,12 @@ importers: specifier: 4.15.1 version: 4.15.1(webpack-cli@5.1.4)(webpack@5.88.1) + mylib: + dependencies: + chalk: + specifier: ^4 + version: 4.1.2 + packages: /@bazel/ibazel@0.16.2: @@ -432,6 +441,13 @@ packages: engines: {node: '>=8'} dev: true + /ansi-styles@4.3.0: + resolution: {integrity: sha512-zbB9rCJAT1rbjiVDb2hqKFHNYLxgtk8NURxZ3IZwD3F6NtxbXZQCnnSi1Lkx+IDohdPlFp222wVALIheZJQSEg==} + engines: {node: '>=8'} + dependencies: + color-convert: 2.0.1 + dev: false + /anymatch@3.1.3: resolution: {integrity: sha512-KMReFUr0B4t+D+OBkjR3KYqvocp2XaSzO55UcB6mgQMd3KbcE+mWTyvVV7D/zsdEbNnV6acZUutkiHQXvTr1Rw==} engines: {node: '>= 8'} @@ -551,6 +567,14 @@ packages: resolution: {integrity: sha512-lQ1VlUUq5q9ro9X+5gOEyH7i3vm+AYVT1WDCVB69XOZ17KZRhnZ9J0Sqz7wTHQaLBJccNCHq8/Ww5LlOIZbB0w==} dev: true + /chalk@4.1.2: + resolution: {integrity: sha512-oKnbhFyRIXpUuez8iBMmyEa4nbj4IOQyuhc/wy9kY7/WVPcwIO9VA668Pu8RkO7+0G76SLROeyw9CpQ061i4mA==} + engines: {node: '>=10'} + dependencies: + ansi-styles: 4.3.0 + supports-color: 7.2.0 + dev: false + /chokidar@3.5.3: resolution: {integrity: sha512-Dr3sfKRP6oTcjf2JmUmFJfeVMvXBdegxB0iVQ5eb2V10uFJUCAS8OByZdVAyVb8xXNz3GjjTgj9kLWsZTqE6kw==} engines: {node: '>= 8.10.0'} @@ -587,6 +611,17 @@ packages: shallow-clone: 3.0.1 dev: true + /color-convert@2.0.1: + resolution: {integrity: sha512-RRECPsj7iu/xb5oKYcsFHSppFNnsj/52OVTRKb4zP5onXwVF3zVmmToNcOfGC+CRDpfK/U584fMg38ZHCaElKQ==} + engines: {node: '>=7.0.0'} + dependencies: + color-name: 1.1.4 + dev: false + + /color-name@1.1.4: + resolution: {integrity: sha512-dOy+3AuW3a2wNbZHIuMZpTcgjGuLU/uBL/ubcZF9OXbDo8ff4O8yVp5Bf0efS8uEoYo5q4Fx7dY9OgQGXgAsQA==} + dev: false + /colorette@2.0.19: resolution: {integrity: sha512-3tlv/dIP7FWvj3BsbHrGLJ6l/oKh1O3TcgBqMn+yyCagOxc23fyzDS6HypQbgxWbkpDnf52p1LuR4eWDQ/K9WQ==} dev: true @@ -1063,7 +1098,6 @@ packages: /has-flag@4.0.0: resolution: {integrity: sha512-EykJT/Q1KjTWctppgIAgfSO0tKVuZUjhgMr17kqTumMl6Afv3EISleU7qZUzoXDFTAHTDC4NOoG/ZxU3EvlMPQ==} engines: {node: '>=8'} - dev: true /has-symbols@1.0.3: resolution: {integrity: sha512-l3LCuF6MgDNwTDKkdYGEihYjt5pRPbEg46rtlmnSPlUbgmB8LOIrKJbYYFBSbnPaJexMKtiPO8hmeRjRz2Td+A==} @@ -1988,6 +2022,13 @@ packages: engines: {node: '>=6'} dev: true + /supports-color@7.2.0: + resolution: {integrity: sha512-qpCAvRl9stuOHveKsn7HncJRvv501qIacKzQlO/+Lwxc9+0q2wLyv4Dfvt80/DPn2pqOBsJdDiogXGR9+OvwRw==} + engines: {node: '>=8'} + dependencies: + has-flag: 4.0.0 + dev: false + /supports-color@8.1.1: resolution: {integrity: sha512-MpUEN2OodtUzxvKQl72cUF7RQ5EiHsGvSsVG0ia9c5RbWGL2CI4C7EpPS8UTBIplnlzZiNuV56w+FuNxy3ty2Q==} engines: {node: '>=10'} diff --git a/e2e/webpack_devserver/pnpm-workspace.yaml b/e2e/webpack_devserver/pnpm-workspace.yaml index 2cce0eb74..3d432df1c 100644 --- a/e2e/webpack_devserver/pnpm-workspace.yaml +++ b/e2e/webpack_devserver/pnpm-workspace.yaml @@ -1,2 +1,3 @@ packages: - '.' + - 'mylib' diff --git a/e2e/webpack_devserver/serve_test.sh b/e2e/webpack_devserver/serve_test.sh index eeca7be8b..acbac73f3 100755 --- a/e2e/webpack_devserver/serve_test.sh +++ b/e2e/webpack_devserver/serve_test.sh @@ -14,15 +14,20 @@ _sedi() { sed "${sedi[@]}" "$@" } +echo "" +echo "" echo "TEST - $0: $1" -./node_modules/.bin/ibazel run "$1" "$BZLMOD_FLAG" >/dev/null 2>&1 & +ibazel_logs=$(mktemp) +echo "Capturing ibazel logs to $ibazel_logs" +./node_modules/.bin/ibazel run "$1" "$BZLMOD_FLAG" >"$ibazel_logs" 2>&1 & ibazel_pid="$!" function _exit { - echo "Cleanup..." kill "$ibazel_pid" - git checkout src/index.html + git checkout src/index.html >/dev/null 2>&1 + git checkout mylib/index.js >/dev/null 2>&1 + rm -f "$ibazel_logs" } trap _exit EXIT @@ -43,6 +48,11 @@ if ! curl http://localhost:8080/index.html --fail 2>/dev/null | grep "Getting St exit 1 fi +if ! curl http://localhost:8080/main.js --fail 2>/dev/null | grep "chalk.blue(packageJson.name)"; then + echo "ERROR: Expected http://localhost:8080/main.js to contain 'chalk.blue(packageJson.name)'" + exit 1 +fi + _sedi 's#Getting Started#Goodbye#' src/index.html echo "Waiting 5 seconds for ibazel rebuild after change to src/index.html..." @@ -53,4 +63,46 @@ if ! curl http://localhost:8080/index.html --fail 2>/dev/null | grep "Goodbye"; exit 1 fi +_sedi 's#blue#red#' mylib/index.js + +echo "Waiting 5 seconds for ibazel rebuild after change to mylib/index.js..." +sleep 5 + +if ! curl http://localhost:8080/main.js --fail 2>/dev/null | grep "chalk.red(packageJson.name)"; then + echo "ERROR: Expected http://localhost:8080/main.js to contain 'chalk.red(packageJson.name)'" + exit 1 +fi + +echo "Checking log file $ibazel_logs" + +count=$(grep -c "Syncing file node_modules/.aspect_rules_js/@mycorp+mylib@0.0.0/node_modules/@mycorp/mylib/index.js" "$ibazel_logs" || true) +if [[ "$count" -ne 2 ]]; then + echo "ERROR: expected to have synced @mycorp/mylib/index.js 2 times but found ${count}" + exit 1 +fi + +count=$(grep -c "Skipping file node_modules/.aspect_rules_js/@mycorp+mylib@0.0.0/node_modules/@mycorp/mylib/index.js since its timestamp has not changed" "$ibazel_logs" || true) +if [[ "$count" -ne 1 ]]; then + echo "ERROR: expected to have skipped @mycorp/mylib/index.js due to timestamp 1 time but found ${count}" + exit 1 +fi + +count=$(grep -c "Syncing file node_modules/.aspect_rules_js/@mycorp+mylib@0.0.0/node_modules/@mycorp/mylib/package.json" "$ibazel_logs" || true) +if [[ "$count" -ne 1 ]]; then + echo "ERROR: expected to have synced @mycorp/mylib/package.json 1 time but found ${count}" + exit 1 +fi + +count=$(grep -c "Skipping file node_modules/.aspect_rules_js/@mycorp+mylib@0.0.0/node_modules/@mycorp/mylib/package.json since its timestamp has not changed" "$ibazel_logs" || true) +if [[ "$count" -ne 1 ]]; then + echo "ERROR: expected to have skipped @mycorp/mylib/package.json due to timestamp 1 time but found ${count}" + exit 1 +fi + +count=$(grep -c "Skipping file node_modules/.aspect_rules_js/@mycorp+mylib@0.0.0/node_modules/@mycorp/mylib/package.json since contents have not changed" "$ibazel_logs" || true) +if [[ "$count" -ne 1 ]]; then + echo "ERROR: expected to have skipped @mycorp/mylib/package.json due to contents 1 time but found ${count}" + exit 1 +fi + echo "All tests passed" diff --git a/e2e/webpack_devserver/src/index.js b/e2e/webpack_devserver/src/index.js index 66af70ed4..282f86a5f 100644 --- a/e2e/webpack_devserver/src/index.js +++ b/e2e/webpack_devserver/src/index.js @@ -1,10 +1,11 @@ import _ from 'lodash' +import { name } from '@mycorp/mylib' function component() { const element = document.createElement('div') // Lodash, currently included via a script, is required for this line to work - element.innerHTML = _.join(['Hello', 'webpack'], ' ') + element.innerHTML = _.join(['Hello', 'webpack', name()], ' ') return element } diff --git a/e2e/webpack_devserver_esm/.bazelignore b/e2e/webpack_devserver_esm/.bazelignore index b512c09d4..262c043e8 100644 --- a/e2e/webpack_devserver_esm/.bazelignore +++ b/e2e/webpack_devserver_esm/.bazelignore @@ -1 +1,2 @@ -node_modules \ No newline at end of file +node_modules +mylib/node_modules diff --git a/e2e/webpack_devserver_esm/BUILD.bazel b/e2e/webpack_devserver_esm/BUILD.bazel index 4ed5e33eb..fb0803222 100644 --- a/e2e/webpack_devserver_esm/BUILD.bazel +++ b/e2e/webpack_devserver_esm/BUILD.bazel @@ -28,5 +28,6 @@ js_run_devserver( "webpack.config.mjs", ":node_modules", ], + log_level = "debug", tool = "webpack_binary", ) diff --git a/e2e/webpack_devserver_esm/mylib/BUILD.bazel b/e2e/webpack_devserver_esm/mylib/BUILD.bazel new file mode 100644 index 000000000..428fe4988 --- /dev/null +++ b/e2e/webpack_devserver_esm/mylib/BUILD.bazel @@ -0,0 +1,10 @@ +load("@aspect_rules_js//npm:defs.bzl", "npm_package") + +npm_package( + name = "mylib", + srcs = [ + "index.js", + "package.json", + ], + visibility = ["//visibility:public"], +) diff --git a/e2e/webpack_devserver_esm/mylib/index.js b/e2e/webpack_devserver_esm/mylib/index.js new file mode 100644 index 000000000..0a7aa03c5 --- /dev/null +++ b/e2e/webpack_devserver_esm/mylib/index.js @@ -0,0 +1,5 @@ +import packageJson from './package.json' +import chalk from 'chalk' +export function name() { + return chalk.blue(packageJson.name) +} diff --git a/e2e/webpack_devserver_esm/mylib/package.json b/e2e/webpack_devserver_esm/mylib/package.json new file mode 100644 index 000000000..b356402d8 --- /dev/null +++ b/e2e/webpack_devserver_esm/mylib/package.json @@ -0,0 +1,6 @@ +{ + "name": "@mycorp/mylib", + "dependencies": { + "chalk": "^4" + } +} diff --git a/e2e/webpack_devserver_esm/package.json b/e2e/webpack_devserver_esm/package.json index 04094b470..df1ae0ecc 100644 --- a/e2e/webpack_devserver_esm/package.json +++ b/e2e/webpack_devserver_esm/package.json @@ -8,6 +8,7 @@ "@bazel/ibazel": "0.16.2" }, "dependencies": { - "lodash": "4.17.21" + "lodash": "4.17.21", + "@mycorp/mylib": "workspace:*" } } diff --git a/e2e/webpack_devserver_esm/pnpm-lock.yaml b/e2e/webpack_devserver_esm/pnpm-lock.yaml index b6f2931cc..bba49f6ef 100644 --- a/e2e/webpack_devserver_esm/pnpm-lock.yaml +++ b/e2e/webpack_devserver_esm/pnpm-lock.yaml @@ -8,6 +8,9 @@ importers: .: dependencies: + '@mycorp/mylib': + specifier: workspace:* + version: link:mylib lodash: specifier: 4.17.21 version: 4.17.21 @@ -28,6 +31,12 @@ importers: specifier: 4.15.1 version: 4.15.1(webpack-cli@5.1.4)(webpack@5.88.1) + mylib: + dependencies: + chalk: + specifier: ^4 + version: 4.1.2 + packages: /@bazel/ibazel@0.16.2: @@ -432,6 +441,13 @@ packages: engines: {node: '>=8'} dev: true + /ansi-styles@4.3.0: + resolution: {integrity: sha512-zbB9rCJAT1rbjiVDb2hqKFHNYLxgtk8NURxZ3IZwD3F6NtxbXZQCnnSi1Lkx+IDohdPlFp222wVALIheZJQSEg==} + engines: {node: '>=8'} + dependencies: + color-convert: 2.0.1 + dev: false + /anymatch@3.1.3: resolution: {integrity: sha512-KMReFUr0B4t+D+OBkjR3KYqvocp2XaSzO55UcB6mgQMd3KbcE+mWTyvVV7D/zsdEbNnV6acZUutkiHQXvTr1Rw==} engines: {node: '>= 8'} @@ -551,6 +567,14 @@ packages: resolution: {integrity: sha512-lQ1VlUUq5q9ro9X+5gOEyH7i3vm+AYVT1WDCVB69XOZ17KZRhnZ9J0Sqz7wTHQaLBJccNCHq8/Ww5LlOIZbB0w==} dev: true + /chalk@4.1.2: + resolution: {integrity: sha512-oKnbhFyRIXpUuez8iBMmyEa4nbj4IOQyuhc/wy9kY7/WVPcwIO9VA668Pu8RkO7+0G76SLROeyw9CpQ061i4mA==} + engines: {node: '>=10'} + dependencies: + ansi-styles: 4.3.0 + supports-color: 7.2.0 + dev: false + /chokidar@3.5.3: resolution: {integrity: sha512-Dr3sfKRP6oTcjf2JmUmFJfeVMvXBdegxB0iVQ5eb2V10uFJUCAS8OByZdVAyVb8xXNz3GjjTgj9kLWsZTqE6kw==} engines: {node: '>= 8.10.0'} @@ -587,6 +611,17 @@ packages: shallow-clone: 3.0.1 dev: true + /color-convert@2.0.1: + resolution: {integrity: sha512-RRECPsj7iu/xb5oKYcsFHSppFNnsj/52OVTRKb4zP5onXwVF3zVmmToNcOfGC+CRDpfK/U584fMg38ZHCaElKQ==} + engines: {node: '>=7.0.0'} + dependencies: + color-name: 1.1.4 + dev: false + + /color-name@1.1.4: + resolution: {integrity: sha512-dOy+3AuW3a2wNbZHIuMZpTcgjGuLU/uBL/ubcZF9OXbDo8ff4O8yVp5Bf0efS8uEoYo5q4Fx7dY9OgQGXgAsQA==} + dev: false + /colorette@2.0.19: resolution: {integrity: sha512-3tlv/dIP7FWvj3BsbHrGLJ6l/oKh1O3TcgBqMn+yyCagOxc23fyzDS6HypQbgxWbkpDnf52p1LuR4eWDQ/K9WQ==} dev: true @@ -1063,7 +1098,6 @@ packages: /has-flag@4.0.0: resolution: {integrity: sha512-EykJT/Q1KjTWctppgIAgfSO0tKVuZUjhgMr17kqTumMl6Afv3EISleU7qZUzoXDFTAHTDC4NOoG/ZxU3EvlMPQ==} engines: {node: '>=8'} - dev: true /has-symbols@1.0.3: resolution: {integrity: sha512-l3LCuF6MgDNwTDKkdYGEihYjt5pRPbEg46rtlmnSPlUbgmB8LOIrKJbYYFBSbnPaJexMKtiPO8hmeRjRz2Td+A==} @@ -1988,6 +2022,13 @@ packages: engines: {node: '>=6'} dev: true + /supports-color@7.2.0: + resolution: {integrity: sha512-qpCAvRl9stuOHveKsn7HncJRvv501qIacKzQlO/+Lwxc9+0q2wLyv4Dfvt80/DPn2pqOBsJdDiogXGR9+OvwRw==} + engines: {node: '>=8'} + dependencies: + has-flag: 4.0.0 + dev: false + /supports-color@8.1.1: resolution: {integrity: sha512-MpUEN2OodtUzxvKQl72cUF7RQ5EiHsGvSsVG0ia9c5RbWGL2CI4C7EpPS8UTBIplnlzZiNuV56w+FuNxy3ty2Q==} engines: {node: '>=10'} diff --git a/e2e/webpack_devserver_esm/pnpm-workspace.yaml b/e2e/webpack_devserver_esm/pnpm-workspace.yaml index 2cce0eb74..3d432df1c 100644 --- a/e2e/webpack_devserver_esm/pnpm-workspace.yaml +++ b/e2e/webpack_devserver_esm/pnpm-workspace.yaml @@ -1,2 +1,3 @@ packages: - '.' + - 'mylib' diff --git a/e2e/webpack_devserver_esm/serve_test.sh b/e2e/webpack_devserver_esm/serve_test.sh index eeca7be8b..c1abdfc09 100755 --- a/e2e/webpack_devserver_esm/serve_test.sh +++ b/e2e/webpack_devserver_esm/serve_test.sh @@ -16,13 +16,16 @@ _sedi() { echo "TEST - $0: $1" -./node_modules/.bin/ibazel run "$1" "$BZLMOD_FLAG" >/dev/null 2>&1 & +ibazel_logs=$(mktemp) +echo "Capturing ibazel logs to $ibazel_logs" +./node_modules/.bin/ibazel run "$1" "$BZLMOD_FLAG" >"$ibazel_logs" 2>&1 & ibazel_pid="$!" function _exit { - echo "Cleanup..." kill "$ibazel_pid" - git checkout src/index.html + git checkout src/index.html >/dev/null 2>&1 + git checkout mylib/index.js >/dev/null 2>&1 + rm -f "$ibazel_logs" } trap _exit EXIT @@ -43,6 +46,11 @@ if ! curl http://localhost:8080/index.html --fail 2>/dev/null | grep "Getting St exit 1 fi +if ! curl http://localhost:8080/main.js --fail 2>/dev/null | grep "chalk__WEBPACK_IMPORTED_MODULE_1___default().blue(_package_json__WEBPACK_IMPORTED_MODULE_0__.name)"; then + echo "ERROR: http://localhost:8080/main.js to contain 'chalk__WEBPACK_IMPORTED_MODULE_1___default().blue(_package_json__WEBPACK_IMPORTED_MODULE_0__.name)'" + exit 1 +fi + _sedi 's#Getting Started#Goodbye#' src/index.html echo "Waiting 5 seconds for ibazel rebuild after change to src/index.html..." @@ -53,4 +61,44 @@ if ! curl http://localhost:8080/index.html --fail 2>/dev/null | grep "Goodbye"; exit 1 fi +_sedi 's#blue#red#' mylib/index.js + +echo "Waiting 5 seconds for ibazel rebuild after change to mylib/index.js..." +sleep 5 + +if ! curl http://localhost:8080/main.js --fail 2>/dev/null | grep "chalk__WEBPACK_IMPORTED_MODULE_1___default().red(_package_json__WEBPACK_IMPORTED_MODULE_0__.name)"; then + echo "ERROR: Expected http://localhost:8080/main.js to contain 'chalk__WEBPACK_IMPORTED_MODULE_1___default().red(_package_json__WEBPACK_IMPORTED_MODULE_0__.name)'" + exit 1 +fi + +count=$(grep -c "Syncing file node_modules/.aspect_rules_js/@mycorp+mylib@0.0.0/node_modules/@mycorp/mylib/index.js" "$ibazel_logs" || true) +if [[ "$count" -ne 2 ]]; then + echo "ERROR: expected to have synced @mycorp/mylib/index.js 2 times but found ${count}" + exit 1 +fi + +count=$(grep -c "Skipping file node_modules/.aspect_rules_js/@mycorp+mylib@0.0.0/node_modules/@mycorp/mylib/index.js since its timestamp has not changed" "$ibazel_logs" || true) +if [[ "$count" -ne 1 ]]; then + echo "ERROR: expected to have skipped @mycorp/mylib/index.js due to timestamp 1 time but found ${count}" + exit 1 +fi + +count=$(grep -c "Syncing file node_modules/.aspect_rules_js/@mycorp+mylib@0.0.0/node_modules/@mycorp/mylib/package.json" "$ibazel_logs" || true) +if [[ "$count" -ne 1 ]]; then + echo "ERROR: expected to have synced @mycorp/mylib/package.json 1 time but found ${count}" + exit 1 +fi + +count=$(grep -c "Skipping file node_modules/.aspect_rules_js/@mycorp+mylib@0.0.0/node_modules/@mycorp/mylib/package.json since its timestamp has not changed" "$ibazel_logs" || true) +if [[ "$count" -ne 1 ]]; then + echo "ERROR: expected to have skipped @mycorp/mylib/package.json due to timestamp 1 time but found ${count}" + exit 1 +fi + +count=$(grep -c "Skipping file node_modules/.aspect_rules_js/@mycorp+mylib@0.0.0/node_modules/@mycorp/mylib/package.json since contents have not changed" "$ibazel_logs" || true) +if [[ "$count" -ne 1 ]]; then + echo "ERROR: expected to have skipped @mycorp/mylib/package.json due to contents 1 time but found ${count}" + exit 1 +fi + echo "All tests passed" diff --git a/e2e/webpack_devserver_esm/src/index.js b/e2e/webpack_devserver_esm/src/index.js index 66af70ed4..282f86a5f 100644 --- a/e2e/webpack_devserver_esm/src/index.js +++ b/e2e/webpack_devserver_esm/src/index.js @@ -1,10 +1,11 @@ import _ from 'lodash' +import { name } from '@mycorp/mylib' function component() { const element = document.createElement('div') // Lodash, currently included via a script, is required for this line to work - element.innerHTML = _.join(['Hello', 'webpack'], ' ') + element.innerHTML = _.join(['Hello', 'webpack', name()], ' ') return element } diff --git a/js/private/js_run_devserver.bzl b/js/private/js_run_devserver.bzl index 7571bf5f3..4139c7956 100644 --- a/js/private/js_run_devserver.bzl +++ b/js/private/js_run_devserver.bzl @@ -52,8 +52,20 @@ def _js_run_devserver_impl(ctx): # artifacts those symlinks point to (node_modules/.aspect_rules_js/foo@1.2.3/node_modules/foo) data_files = [] for f in depset(transitive = transitive_runfiles + [dep.files for dep in ctx.attr.data]).to_list(): - # don't include the virtual store tree artifact; only the node_module link is needed - if not "/.aspect_rules_js/" in f.path: + if "/.aspect_rules_js/" in f.path: + # Special handling for virtual store deps; we only include 1st party deps since copying + # all 3rd party node_modules over is expensive for typical graphs + path_segments = f.path.split("/") + package_name_segment = path_segments.index(".aspect_rules_js") + 1 + + # TODO: @0.0.0 is by default the version of all 1p linked packages, however, it can be overridden by users + # if they are manually linking a 1p package and not using workspace. A more robust solution would be to + # split handling of 1p and 3p package in the JsInfo provider itself. Other optimizations in the rule set + # could also be made if that was the case. + if len(path_segments) > package_name_segment and "@0.0.0" in path_segments[package_name_segment]: + # include this first party linked dependency + data_files.append(f) + else: data_files.append(f) config = { diff --git a/js/private/js_run_devserver.mjs b/js/private/js_run_devserver.mjs index da9c75add..687f9180a 100644 --- a/js/private/js_run_devserver.mjs +++ b/js/private/js_run_devserver.mjs @@ -3,13 +3,15 @@ import * as perf_hooks from 'node:perf_hooks' import * as fs from 'node:fs' import * as os from 'node:os' import * as child_process from 'node:child_process' +import * as crypto from 'node:crypto' // Globals const RUNFILES_ROOT = path.join( process.env.JS_BINARY__RUNFILES, process.env.JS_BINARY__WORKSPACE ) -const synced = new Map() +const syncedTime = new Map() +const syncedChecksum = new Map() const mkdirs = new Set() // Ensure that a directory exists. If it has not been previously created or does not exist then it @@ -34,13 +36,9 @@ function mkdirpSync(p) { } // Determines if a file path refers to a node module. -// -// Examples: -// isNodeModulePath('/private/var/.../node_modules/@babel/core') // true -// isNodeModulePath('/private/var/.../node_modules/lodash') // true -// isNodeModulePath('/private/var/.../some-file.js') // false -function isNodeModulePath(srcPath) { - const parentDir = path.dirname(srcPath) +// See js/private/test/js_run_devserver/js_run_devserver.spec.mjs for examples. +export function isNodeModulePath(p) { + const parentDir = path.dirname(p) const parentDirName = path.basename(parentDir) if (parentDirName === 'node_modules') { @@ -54,42 +52,122 @@ function isNodeModulePath(srcPath) { return false } +// Determines if a file path is a 1p dep in the virtual store. +// See js/private/test/js_run_devserver/js_run_devserver.spec.mjs for examples. +export function is1pVirtualStoreDep(p) { + // unscoped1p: https://regex101.com/r/hBR08J/1 + const unscoped1p = + /^.+\/\.aspect_rules_js\/([^@\/]+)@0\.0\.0\/node_modules\/\1$/ + // scoped1p: https://regex101.com/r/bWS7Hl/1 + const scoped1p = + /^.+\/\.aspect_rules_js\/@([^@+\/]+)\+([^@+\/]+)@0\.0\.0\/node_modules\/@\1\/\2$/ + return p.match(unscoped1p) || p.match(scoped1p) +} + +// Hashes a file using a read stream. Based on https://github.com/kodie/md5-file. +async function generateChecksum(p) { + return new Promise((resolve, reject) => { + const output = crypto.createHash('md5') + const input = fs.createReadStream(p) + input.on('error', (err) => { + reject(err) + }) + output.once('readable', () => { + resolve(output.read().toString('hex')) + }) + input.pipe(output) + }) +} + +// Converts a size in bytes to a human readable friendly number such as "24 KiB" +export function friendlyFileSize(bytes) { + if (!bytes) { + return '0 B' + } + const e = Math.floor(Math.log(bytes) / Math.log(1024)) + if (e == 0) { + return `${bytes} B` + } + return ( + (bytes / Math.pow(1024, Math.min(e, 5))).toFixed(1) + + ' ' + + ' KMGTP'.charAt(Math.min(e, 5)) + + 'iB' + ) +} + +// https://stackoverflow.com/a/42299191 +function partitionArray(array, callback) { + return array.reduce( + function (result, element, i) { + callback(element, i, array) + ? result[0].push(element) + : result[1].push(element) + + return result + }, + [[], []] + ) +} + // Recursively copies a file, symlink or directory to a destination. If the file has been previously // synced it is only re-copied if the file's last modified time has changed since the last time that // file was copied. Symlinks are not copied but instead a symlink is created under the destination // pointing to the source symlink. -async function syncRecursive(src, dst, writePerm) { +async function syncRecursive(src, dst, sandbox, writePerm) { try { const lstat = await fs.promises.lstat(src) - const last = synced.get(src) + const last = syncedTime.get(src) if (!lstat.isDirectory() && last && lstat.mtimeMs == last) { // this file is already up-to-date + if (process.env.JS_BINARY__LOG_DEBUG) { + console.error( + `Skipping file ${src.slice( + RUNFILES_ROOT.length + 1 + )} since its timestamp has not changed` + ) + } return 0 } - const exists = synced.has(src) || fs.existsSync(dst) - synced.set(src, lstat.mtimeMs) + const exists = syncedTime.has(src) || fs.existsSync(dst) + syncedTime.set(src, lstat.mtimeMs) if (lstat.isSymbolicLink()) { const srcWorkspacePath = src.slice(RUNFILES_ROOT.length + 1) - if (process.env.JS_BINARY__LOG_DEBUG) { - console.error(`Syncing symlink ${srcWorkspacePath}`) - } + let symlinkMeta = '' if (isNodeModulePath(src)) { - // Special case for node_modules symlinks where we should _not_ symlink to the runfiles but rather - // the bin copy of the symlink to avoid finding npm packages in multiple node_modules trees - const maybeBinSrc = path.join( - process.env.JS_BINARY__EXECROOT, - process.env.JS_BINARY__BINDIR, - srcWorkspacePath + let linkPath = await fs.promises.readlink(src) + if (path.isAbsolute(linkPath)) { + linkPath = path.relative(src, linkPath) + } + // Special case for 1p node_modules symlinks + const maybe1pSync = path.normalize( + path.join(sandbox, srcWorkspacePath, linkPath) ) - if (fs.existsSync(maybeBinSrc)) { - if (process.env.JS_BINARY__LOG_DEBUG) { - console.error( - `Syncing to bazel-out copy of symlink ${srcWorkspacePath}` - ) + if (fs.existsSync(maybe1pSync)) { + src = maybe1pSync + symlinkMeta = '1p' + } + if (!symlinkMeta) { + // Special case for node_modules symlinks where we should _not_ symlink to the runfiles but rather + // the bin copy of the symlink to avoid finding npm packages in multiple node_modules trees + const maybeBinSrc = path.join( + process.env.JS_BINARY__EXECROOT, + process.env.JS_BINARY__BINDIR, + srcWorkspacePath + ) + if (fs.existsSync(maybeBinSrc)) { + src = maybeBinSrc + symlinkMeta = 'bazel-out' } - src = maybeBinSrc } } + if (process.env.JS_BINARY__LOG_DEBUG) { + console.error( + `Syncing symlink ${srcWorkspacePath}${ + symlinkMeta ? ` (${symlinkMeta})` : '' + }` + ) + } if (exists) { await fs.promises.unlink(dst) } else { @@ -111,15 +189,33 @@ async function syncRecursive(src, dst, writePerm) { await syncRecursive( path.join(src, entry), path.join(dst, entry), + sandbox, writePerm ) ) ) ).reduce((s, t) => s + t, 0) } else { + const lastChecksum = syncedChecksum.get(src) + const checksum = await generateChecksum(src) + if (lastChecksum && checksum == lastChecksum) { + // the file contents have not changed since the last sync + if (process.env.JS_BINARY__LOG_DEBUG) { + console.error( + `Skipping file ${src.slice( + RUNFILES_ROOT.length + 1 + )} since contents have not changed` + ) + } + return 0 + } + syncedChecksum.set(src, checksum) + if (process.env.JS_BINARY__LOG_DEBUG) { console.error( - `Syncing file ${src.slice(RUNFILES_ROOT.length + 1)}` + `Syncing file ${src.slice( + RUNFILES_ROOT.length + 1 + )} (${friendlyFileSize(lstat.size)})` ) } if (exists) { @@ -128,15 +224,18 @@ async function syncRecursive(src, dst, writePerm) { // Intentionally synchronous; see comment on mkdirpSync mkdirpSync(path.dirname(dst)) } + await fs.promises.copyFile(src, dst) if (writePerm) { const s = await fs.promises.stat(dst) const mode = s.mode | fs.constants.S_IWUSR - console.error( - `Adding write permissions to file ${src.slice( - RUNFILES_ROOT.length + 1 - )}: ${(mode & parseInt('777', 8)).toString(8)}` - ) + if (process.env.JS_BINARY__LOG_DEBUG) { + console.error( + `Adding write permissions to file ${src.slice( + RUNFILES_ROOT.length + 1 + )}: ${(mode & parseInt('777', 8)).toString(8)}` + ) + } await fs.promises.chmod(dst, mode) } return 1 @@ -149,17 +248,48 @@ async function syncRecursive(src, dst, writePerm) { // Sync list of files to the sandbox async function sync(files, sandbox, writePerm) { - console.error('Syncing...') + console.error(`Syncing ${files.length} files && folders...`) const startTime = perf_hooks.performance.now() - const totalSynced = ( + + const [virtualStore1pFiles, remainingFiles] = partitionArray( + files, + is1pVirtualStoreDep + ) + + if (virtualStore1pFiles.length > 0 && process.env.JS_BINARY__LOG_DEBUG) { + console.error( + `Syncing ${virtualStore1pFiles.length} first party virtual store dep(s)` + ) + } + + // Sync first-party virtual store files first since correctly syncing direct 1p node_modules + // symlinks depends on checking if the virtual store synced files exist. + let totalSynced = ( await Promise.all( - files.map(async (file) => { + virtualStore1pFiles.map(async (file) => { const src = path.join(RUNFILES_ROOT, file) const dst = path.join(sandbox, file) - return await syncRecursive(src, dst, writePerm) + return await syncRecursive(src, dst, sandbox, writePerm) }) ) ).reduce((s, t) => s + t, 0) + + if (remainingFiles.length > 0 && process.env.JS_BINARY__LOG_DEBUG) { + console.error( + `Syncing ${remainingFiles.length} other files && folders...` + ) + } + + totalSynced += ( + await Promise.all( + remainingFiles.map(async (file) => { + const src = path.join(RUNFILES_ROOT, file) + const dst = path.join(sandbox, file) + return await syncRecursive(src, dst, sandbox, writePerm) + }) + ) + ).reduce((s, t) => s + t, 0) + var endTime = perf_hooks.performance.now() console.error( `${totalSynced} file${ @@ -285,6 +415,9 @@ async function main(args, sandbox) { } ;(async () => { + if (process.env.__RULES_JS_UNIT_TEST__) + // short-circuit for unit tests + return let sandbox try { sandbox = path.join( diff --git a/js/private/test/js_run_devserver/BUILD b/js/private/test/js_run_devserver/BUILD.bazel similarity index 83% rename from js/private/test/js_run_devserver/BUILD rename to js/private/test/js_run_devserver/BUILD.bazel index 648880e46..1fb7bc5c5 100644 --- a/js/private/test/js_run_devserver/BUILD +++ b/js/private/test/js_run_devserver/BUILD.bazel @@ -1,3 +1,4 @@ +load("@aspect_rules_js//js:defs.bzl", "js_test") load("@npm//:defs.bzl", "npm_link_all_packages") load("@npm//js/private/test/js_run_devserver:jasmine/package_json.bzl", jasmine_bin = "bin") load(":js_run_devserver_test.bzl", "js_run_devserver_test") @@ -32,3 +33,12 @@ js_run_devserver_test( jasmine_bin.jasmine_binary( name = "jasmine", ) + +js_test( + name = "js_run_devserver_test", + data = ["//js/private:js_devserver_entrypoint"], + entry_point = "js_run_devserver.spec.mjs", + env = { + "__RULES_JS_UNIT_TEST__": "1", + }, +) diff --git a/js/private/test/js_run_devserver/js_run_devserver.spec.mjs b/js/private/test/js_run_devserver/js_run_devserver.spec.mjs new file mode 100644 index 000000000..71c103605 --- /dev/null +++ b/js/private/test/js_run_devserver/js_run_devserver.spec.mjs @@ -0,0 +1,80 @@ +import { + isNodeModulePath, + is1pVirtualStoreDep, + friendlyFileSize, +} from '../../js_run_devserver.mjs' + +// isNodeModulePath +const isNodeModulePath_true = [ + '/private/var/some/path/node_modules/@babel/core', + '/private/var/some/path/node_modules/lodash', +] +for (const p of isNodeModulePath_true) { + if (!isNodeModulePath(p)) { + console.error(`ERROR: expected ${p} to be a node_modules path`) + process.exit(1) + } +} +const isNodeModulePath_false = ['/private/var/some/path/some-file.js'] +for (const p of isNodeModulePath_false) { + if (isNodeModulePath(p)) { + console.error(`ERROR: expected ${p} to not be a node_modules path`) + process.exit(1) + } +} + +// is1pVirtualStoreDep +const is1pVirtualStoreDep_true = [ + 'some/path/node_modules/.aspect_rules_js/@mycorp+pkg@0.0.0/node_modules/@mycorp/pkg', + 'some/path/node_modules/.aspect_rules_js/mycorp-pkg@0.0.0/node_modules/mycorp-pkg', +] +for (const p of is1pVirtualStoreDep_true) { + if (!is1pVirtualStoreDep(p)) { + console.error(`ERROR: expected ${p} to be a 1p virtual store dep`) + process.exit(1) + } +} +const is1pVirtualStoreDep_false = [ + 'some/path/node_modules/.aspect_rules_js/@mycorp+pkg@0.0.0/node_modules/mycorp/pkg', + 'some/path/node_modules/.aspect_rules_js/@mycorp+pkg0.0.0/node_modules/@mycorp/pkg', + 'some/path/node_modules/.aspect_rules_js/mycorp+pkg@0.0.0/node_modules/@mycorp/pkg', + 'some/path/node_modules/.aspect_rules_js/mycorp-pkg0.0.0/node_modules/mycorp-pkg', + 'some/path/node_modules/.aspect_rules_js/@mycorp+pkg@0.0.0/node_modules/acorn', + 'some/path/node_modules/.aspect_rules_js/mycorp-pkg@0.0.0/node_modules/acorn', + 'some/path/node_modules/.aspect_rules_js/@babel+runtime@7.21.0/node_modules/@babel/runtime', + 'some/path/node_modules/.aspect_rules_js/@babel+runtime@7.21.0/node_modules/acorn', + 'some/path/node_modules/.aspect_rules_js/eval@0.1.6/node_modules/eval', + 'some/path/node_modules/.aspect_rules_js/eval@0.1.6/node_modules/acorn', +] +for (const p of is1pVirtualStoreDep_false) { + if (is1pVirtualStoreDep(p)) { + console.error(`ERROR: expected ${p} to not be a 1p virtual store dep`) + process.exit(1) + } +} + +// friendlyFileSize +const friendlyFileSize_cases = new Map() +friendlyFileSize_cases.set(0, '0 B') +friendlyFileSize_cases.set(1, '1 B') +friendlyFileSize_cases.set(100, '100 B') +friendlyFileSize_cases.set(1023, '1023 B') +friendlyFileSize_cases.set(1024, '1.0 KiB') +friendlyFileSize_cases.set(1300, '1.3 KiB') +friendlyFileSize_cases.set(1024 * 1024, '1.0 MiB') +friendlyFileSize_cases.set(1024 * 1024 * 1024, '1.0 GiB') +friendlyFileSize_cases.set(1024 * 1024 * 1024 * 1024, '1.0 TiB') +friendlyFileSize_cases.set(1024 * 1024 * 1024 * 1024 * 1024, '1.0 PiB') +friendlyFileSize_cases.set( + 1024 * 1024 * 1024 * 1024 * 1024 * 1024, + '1024.0 PiB' +) +for (const [k, v] of friendlyFileSize_cases) { + const a = friendlyFileSize(k) + if (a !== v) { + console.error( + `Expected friendlyFileSize(${k}) to be '${v}' but got '${a}'` + ) + process.exit(1) + } +}