bundle ng-basic-catalog as default out-of-the-box renderer#71
Conversation
⚡ A2UI Composer PR PreviewThis Pull Request was closed. The preview environment at |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for relative renderer URLs (starting with '/') across the application, updating the configuration, settings validation, query parser, and preview bridge fetch calls accordingly. It also integrates dynamic initial draft pre-population based on the active catalog. The reviewer feedback highlights three main areas for improvement: addressing potential state desynchronization when switching active catalogs, using JSON.stringify instead of template string interpolation to construct JSON safely, and replacing corepack yarn with standard yarn in the package scripts for better portability and consistency.
jgindin
force-pushed
the
fast-start
branch
4 times, most recently
from
2d74a02 to
7826a66
Compare
jgindin
force-pushed
the
fast-start
branch
2 times, most recently
from
428ed8e to
e17b597
Compare
ditman
left a comment
ditman
left a comment
There was a problem hiding this comment.
The way we're symlinking the ng-basic-catalog dist to make it "look like a package" is a little bit iffy. Could it be a yarn workspace package instead? I don't think we'd need to publish it to npm, but at least the dependency in the shell app would be "workspace:*" or similar?
jgindin
force-pushed
the
fast-start
branch
2 times, most recently
from
95465e1 to
78545c6
Compare
ditman
left a comment
ditman
left a comment
There was a problem hiding this comment.
I left some more comments/questions, but I don't want to block you too much. Please address whatever you consider necessary (or create issues to improve later!)
| "angular-eslint": "22.0.0", | ||
| "eslint": "10.5.0", | ||
| "jsdom": "29.1.1", | ||
| "ng-basic-catalog": "0.1.0", |
There was a problem hiding this comment.
I don't think this dependency is needed, since we're globbing node_modules/ng-basic-catalog/dist/ng-basic-catalog/browser as assets above.
(Also, I don't understand how this'd resolve, since ng-basic-catalog is not a real package? I'm clearly missing some yarn sorcery here)
| if (typeof window === 'undefined' || !window.fetch) return null; | ||
|
|
||
| let res = await this.fetchWithTimeout('/catalog'); | ||
| let res = await this.fetchWithTimeout('catalog'); |
There was a problem hiding this comment.
These URLs are now more sensitive to the way the iframe is deployed, for example, doing fetch('bar.json') yields different results when run in:
- https://example.com/foo -> tries to fetch
https://example.com/bar.json - https://example.com/foo/ -> tries to fetch
https://example.com/foo/bar.json- Which is the same as form https://example.com/foo/index.html
I guess this is acceptable, but it may be better to create a new URL for these resources that resolves against a more specific window.location base URL (similar to what was done below), or make a specific note about this deployment requirement in the manual of the preview bridge.
- Update shell build options in angular.json to copy basic catalog assets from the locally resolved node_modules folder. - Enforce topological order and exclude root workspace in root package.json scripts (build, test, prettier, clean, lint) to prevent execution loops. - Reorder build steps in deploy workflows to build ng-basic-catalog before the shell. - Set defaultRendererUrl in config.json to relative path. - Add basic catalog workspace dependency to shell package.json for build ordering, and build catalog workspace on shell start. - Configure ng-basic-catalog build with relative base href to support embedded frame loading. - Resolve preview-bridge catalog endpoints relatively to support embedded paths. - Relax settings page pattern validator and QueryParser URL parser to support relative paths. - Add E2E and unit tests verifying relative URL resolution, validation, and initial workspace loading. - Restrict CAR_BOOKING pre-population to when the renderer app supports the basic catalog. - Update StateSync to dynamically detect catalog changes and reset the active draft if incompatible. - Add unit tests for StateSync to cover dynamic draft resets on catalog change and verify parsing logic edge cases. TAG=agy BUG=
2 participants
Description
Include the ng-basic-catalog sample in with the shell build so that you can be up & running immediately.
Pre-launch Checklist