Skip to content

Chore: Upgrade Node.js, improve publishing scripts, and fix Angular tests#1177

Open
jacobsimionato wants to merge 4 commits intogoogle:mainfrom
jacobsimionato:another_improve_script
Open

Chore: Upgrade Node.js, improve publishing scripts, and fix Angular tests#1177
jacobsimionato wants to merge 4 commits intogoogle:mainfrom
jacobsimionato:another_improve_script

Conversation

@jacobsimionato
Copy link
Copy Markdown
Collaborator

Description of Changes

This PR introduces several improvements to the build, test, and publishing infrastructure:

  • Node.js Upgrade: Updated Node.js version from 20 to 22 across all GitHub Action workflows.
  • Improved Publishing Process:
    • Refactored prepare-publish.mjs to dynamically update all internal @a2ui/ dependencies (using file: links) to their current workspace versions. This replaces the previous hardcoded logic that only handled @a2ui/web_core.
    • Updated publish_npm.mjs safety checks to ensure @a2ui/markdown-it is also published alongside renderers, preventing version mismatches for shared dependencies.
    • Updated integration tests in publish_npm.test.mjs to verify the new topological sorting and publishing flow including markdown-it.
  • Angular Renderer Enhancements:
    • Added a dedicated CI test step for the Angular renderer.
    • Fixed ColumnComponent and RowComponent unit tests to correctly verify styles on the host element and updated expected flex values to align with standard CSS (e.g., flex-start).
  • Web Core Fix: Updated the test command in @a2ui/web_core to correctly target compiled test files in the dist/ directory.

Rationale

  • Upgrading to Node.js 22 ensures we are using a modern, supported environment with better performance.
  • The publishing script improvements make the mono-repo management more robust and less error-prone by automating dependency resolution for all internal packages.
  • Adding Angular tests to CI and fixing the existing tests improves the reliability of the Angular renderer.

Testing/Running Instructions

  1. Workflows: Verify that CI runs successfully on this PR with Node 22.
  2. Angular Tests:
    cd renderers/angular
    npm install
    npm run test:ci
  3. Web Core Tests:
    cd renderers/web_core
    npm install
    npm run build
    npm test
  4. Publishing Scripts:
    The integration tests for the publishing scripts can be run via:
    cd renderers
    node --test scripts/publish_npm.test.mjs

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the package preparation and publishing scripts to dynamically resolve internal @a2ui/ dependencies via a workspace graph and extends publishing safety checks to include @a2ui/markdown-it. Additionally, it updates Angular component tests to verify styles on the host element and refines the web_core test execution command. Review feedback recommends using more idiomatic JavaScript for dependency updates, including optionalDependencies in the process, and dynamically identifying packages for safety checks to eliminate hardcoded lists.

Comment on lines +53 to +61
const updateInternalDeps = (deps) => {
if (!deps) return;
for (const name in deps) {
const version = deps[name];
if (version.startsWith('file:') && graph[name]) {
deps[name] = '^' + graph[name].version;
}
}
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

Refactoring updateInternalDeps to use Object.entries is cleaner and more idiomatic in modern JavaScript. Additionally, throwing an error when a file: dependency cannot be resolved in the workspace graph prevents the accidental publication of broken packages that contain unresolved local links.

Suggested change
const updateInternalDeps = (deps) => {
if (!deps) return;
for (const name in deps) {
const version = deps[name];
if (version.startsWith('file:') && graph[name]) {
deps[name] = '^' + graph[name].version;
}
}
};
const updateInternalDeps = (deps) => {
if (!deps) return;
for (const [name, version] of Object.entries(deps)) {
if (typeof version === 'string' && version.startsWith('file:')) {
if (graph[name]) {
deps[name] = '^' + graph[name].version;
} else {
throw new Error(`Internal dependency ${name} not found in workspace.`);
}
}
}
};

Comment on lines +63 to +64
updateInternalDeps(pkg.dependencies);
updateInternalDeps(pkg.peerDependencies);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The script should also update internal dependencies in optionalDependencies to ensure they are correctly resolved when the package is published to npm. Currently, only dependencies and peerDependencies are handled.

Suggested change
updateInternalDeps(pkg.dependencies);
updateInternalDeps(pkg.peerDependencies);
updateInternalDeps(pkg.dependencies);
updateInternalDeps(pkg.peerDependencies);
updateInternalDeps(pkg.optionalDependencies);

Comment on lines 68 to 69
const renderers = ['@a2ui/lit', '@a2ui/angular', '@a2ui/react'];
const requestedRenderers = resolvedPackages.filter(p => renderers.includes(p));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Instead of hardcoding the list of renderers, you can dynamically identify packages that depend on core libraries (like @a2ui/web_core or @a2ui/markdown-it) using the package graph. This makes the safety check more robust as new packages are added to the mono-repo.

  const corePackages = [webCoreName, markdownItName];
  const requestedRenderers = resolvedPackages.filter(name => {
    const pkg = graph[name];
    return pkg && pkg.internalDependencies.some(dep => corePackages.includes(dep));
  });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

1 participant