Render scripts from Script Manager#2477
Conversation
🦋 Changeset detectedLatest commit: 292574f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
There was a problem hiding this comment.
Pull Request Overview
Adds a new mechanism to fetch and render BigCommerce Script Manager scripts in the Catalyst storefront using Next.js’s <Script> component.
- Introduces
ScriptManagerScriptsto process and render inline (InlineScript) and external (SrcScript) scripts with proper strategies and integrity support. - Defines
ScriptsFragmentto retrieve header and footer scripts via GraphQL and integrates it into the root layout. - Updates documentation, GraphQL type mappings, and adds a changelog entry.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| core/components/scripts/scripts.tsx | Implements ScriptManagerScripts and script rendering |
| core/components/scripts/fragment.ts | Adds GraphQL fragment for header and footer scripts |
| core/components/scripts/README.md | Documents usage and component behavior |
| core/components/scripts/index.tsx | Exports component and fragment |
| core/client/graphql.ts | Adds UUID to GraphQL scalar mappings |
| core/app/[locale]/layout.tsx | Integrates ScriptManagerScripts into the layout |
| .changeset/wise-ads-drive.md | Changelog entry for script rendering feature |
Comments suppressed due to low confidence (2)
core/components/scripts/scripts.tsx:16
- No tests were added for
ScriptManagerScripts. Consider adding unit tests to verify handling of external and inline scripts, including integrity hash anddangerouslySetInnerHTMLscenarios.
export const ScriptManagerScripts = ({ scripts, strategy }: ScriptRendererProps) => {
core/components/scripts/README.md:10
- Add a note that consumers must include
...ScriptsFragmentin their GraphQL queries (headerScripts/footerScripts) to supply data toScriptManagerScripts.
## Usage
| export const ScriptsFragment = graphql(` | ||
| fragment ScriptsFragment on Content { | ||
| headerScripts: scripts( | ||
| first: 50 | ||
| filters: { visibilities: [ALL_PAGES, STOREFRONT], location: HEAD } | ||
| ) { | ||
| ...ScriptTypeConnectionFragment | ||
| } | ||
| footerScripts: scripts( | ||
| first: 50 |
There was a problem hiding this comment.
[nitpick] The hardcoded limit of first: 50 may truncate scripts if more are added. Consider extracting this into a named constant or documenting the rationale for using 50.
| export const ScriptsFragment = graphql(` | |
| fragment ScriptsFragment on Content { | |
| headerScripts: scripts( | |
| first: 50 | |
| filters: { visibilities: [ALL_PAGES, STOREFRONT], location: HEAD } | |
| ) { | |
| ...ScriptTypeConnectionFragment | |
| } | |
| footerScripts: scripts( | |
| first: 50 | |
| // Default limit for the number of scripts to fetch. Adjust this value as needed. | |
| const DEFAULT_SCRIPT_LIMIT = 50; | |
| export const ScriptsFragment = graphql(` | |
| fragment ScriptsFragment on Content { | |
| headerScripts: scripts( | |
| first: DEFAULT_SCRIPT_LIMIT | |
| filters: { visibilities: [ALL_PAGES, STOREFRONT], location: HEAD } | |
| ) { | |
| ...ScriptTypeConnectionFragment | |
| } | |
| footerScripts: scripts( | |
| first: DEFAULT_SCRIPT_LIMIT |
chanceaclark
left a comment
There was a problem hiding this comment.
The dom-purify is the only major thing, otherwise looks good to me.
| // Handle inline scripts (InlineScript) | ||
| if (script.__typename === 'InlineScript' && script.scriptTag) { | ||
| const scriptMatch = /<script[^>]*>([\s\S]*?)<\/script>/i.exec(script.scriptTag); | ||
|
|
||
| if (scriptMatch?.[1]) { | ||
| scriptProps.dangerouslySetInnerHTML = { | ||
| __html: scriptMatch[1], | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we should have dom-purify here in the repo that you might want to use instead of a regex 🤔
|
|
||
| return ( | ||
| <> | ||
| {scriptNodes.map((script) => { |
There was a problem hiding this comment.
🍹 Only if you want to update this... but a better practice, which might seem weird, is to do two .map's here. One to build the props, then another one to render. It allows each .map to have separate concerns.
{scriptNodes
.map((script) => {
// ...
return {
src: "...",
// ....
};
})
.map((script) => {
return (
<Script key={script.id} {...script} />
);
})
}…rtal (#2504) * Version Packages (`canary`) (#2452) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Bump next to latest canary (#2465) * docs: how to verify new merge base established when syncing (#2451) Improve explanation around the importance of establishing a new merge base every time `canary` is merged into `integrations/makeswift` * chore(core): remove runtime edge declarations (#2464) * Update translations (#2468) * feat(other): LOCAL-1444 delivery translation * chore(core): create translations patch --------- Co-authored-by: bc-svc-local <bc-svc-local@users.noreply.github.com> Co-authored-by: Jorge Moya <jorge.moya@bigcommerce.com> * Use min and max purchase quantity to set bounds of quantity number input (#2474) * chore: update ci to include @bigcommerce/catalyst (#2463) * chore: update deps mostly related to packages/cli (#2461) * chore: use @commander-js/extra-typings as devDep (#2462) * feat: render scripts from Script Manager (#2477) * chore: hoist eslint plugins for ide support (#2460) * feat: recursively copy cwd into temp folder (#2479) * feat(cli): scaffold deploy command and generate bundle zip (#2481) * feat(cli): scaffold deploy command and generate bundle zip * feat: remove archiver and only use adm-zip * feat: add extra checks for build files * refactor: move checks around * fix: use .bigcommerce/dist as build path * refactor: split mkTempDir to own lib file and update tests * fix: remove file * fix: set default value for prefix * fix: remove extra-typings * feat: install and build all deps required to bundle catalyst (#2486) * chore: bump opennextjs-cloudflare version (#2487) * feat(cli): generate upload signature and upload bundle.zip (#2484) * feat(cli): generate upload signature and upload bundle.zip * feat: read from env variables * feat: use zod to validate response * refactor: parse * feat: read api host from env * refactor: infer option types in deploy command action (#2490) * feat: copy templates from cli to catalyst build tmpdir (#2489) * Version Packages (`canary`) (#2466) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Nathan Booker <bookernath@users.noreply.github.com> Co-authored-by: Matthew Volk <matt.volk@bigcommerce.com> Co-authored-by: Jorge Moya <jorge.moya@bigcommerce.com> Co-authored-by: bc-svc-local <102379007+bc-svc-local@users.noreply.github.com> Co-authored-by: bc-svc-local <bc-svc-local@users.noreply.github.com>
What/Why?
Checks for scripts in Script Manager tied to the Catalyst storefront's channel and renders them in the root layout using next/script.
Related docs PR: https://github.com/bigcommerce/docs/pull/1014
Future work will add a consent manager into Catalyst which will apply to both scripts as well as analytics calls, respecting cookie consent automatically.
Testing
Preview deployment
Migration
N/A