Browser Installation notes#40
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates Tempo's browser installation guidance across README and docs to present Global Bundle, Smart CDN (esm.sh), and Granular ESM approaches with examples updated to Tempo v3 and newer polyfill pins; the importmap adds two new module entries to support granular subpaths and plugin modules. ChangesBrowser Distribution Documentation and Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tempo/importmap.json (1)
3-11:⚠️ Potential issue | 🟡 MinorDocument hosting expectations for
packages/tempo/importmap.json(and the#tempo/licenseinternal mapping).
packages/tempo/doc/migration-guide.mdcalls the shippedpackages/tempo/importmap.jsonthe sanctioned browser reference and notes it’s the supported exception to hand-authoringdist/paths, so the mixed CDN URLs (lines 3-4) and./dist/...mappings (lines 5-11, incl.#tempo/license) looks intentional. Add a brief top-of-file comment stating that the./dist/...entries must resolve relative to where this JSON is hosted (e.g., alongside the package’sdist/), and clarify that#tempo/licenseis an internal/private specifier mapping for runtime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/importmap.json` around lines 3 - 11, Add a short top-of-file comment in packages/tempo/importmap.json stating that the JSON is the sanctioned browser import map for this package, that any "./dist/..." entries (e.g., "`@magmacomputing/tempo/core`", "…/duration", "…/mutate", "…/plugin", "…/library", "…/enums") must resolve relative to where this JSON is hosted (typically alongside the package's dist/), and explicitly note that "`#tempo/license`" is an internal/private specifier used at runtime; place this comment at the very top so readers understand the mixed CDN vs local ./dist resolution expectations.
🧹 Nitpick comments (2)
packages/tempo/README.md (1)
74-74: 💤 Low valueHeading level skip is flagged but appears intentional.
The static analysis tool flagged this h4 heading after what it perceives as an h2 (the "Installation" section at line 56). However, this structure is consistent with the sibling headings at lines 88, 101, and 130, which are all h4s representing sub-options within the collapsible "Browser & Native Environments" section.
If this heading hierarchy is intentional (h4s as option numbers within the expandable details), you can suppress the linter warning. Otherwise, consider using h3 (###) for these numbered options.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/README.md` at line 74, The h4 heading "#### 1. The Global Bundle (Standard Usage)" under the "Browser & Native Environments" collapsible uses h4s for numbered options and is being flagged for skipping heading levels; either change this and the sibling option headings ("#### 2. ...", "#### 3. ...", etc.) to h3 (###) to maintain proper hierarchical order, or add a linter suppression comment/configuration for this intentional structure so the static analysis ignores these h4 option headings.Source: Linters/SAST tools
packages/tempo/importmap.json (1)
11-11: 💤 Low valueNote: Unconventional use of private specifier pattern in public import map.
The
#tempo/licensespecifier uses the Node.js private import convention (#prefix), which is typically reserved for package-internal subpath imports. While this is technically valid in Import Maps and appears to be an intentional design choice to keep the licensing module scoped, it's unconventional to expose a private-prefixed specifier in a public import map.This pattern works correctly and is consistently documented in the installation guide. However, developers unfamiliar with this approach might find it unexpected when consuming the library.
Consider adding a brief comment in the relevant documentation explaining why the private specifier pattern is used here, if not already present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/importmap.json` at line 11, The import map exposes a private-style specifier "`#tempo/license`" which is unconventional for public import maps; update the project's documentation (e.g., installation guide or package README) to include a brief note explaining why the Node.js private-specifier pattern (the "`#tempo/license`" entry in importmap.json) is intentionally used and how consumers should interpret it so unfamiliar developers are not confused.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tempo/doc/installation.md`:
- Line 163: The dependency entry for "`@magmacomputing/tempo-plugin-astro`" in the
installation doc is using version "1.0.0" which conflicts with the README.md
granular ESM example that uses "1.1.6"; determine the correct target version
(confirm whether 1.1.6 is intended), then update the string literal
"`@magmacomputing/tempo-plugin-astro`":
"https://cdn.jsdelivr.net/npm/@magmacomputing/tempo-plugin-astro@1.0.0/dist/index.js"
to use the verified version and make the same change in the README example so
both documents reference the identical version.
---
Outside diff comments:
In `@packages/tempo/importmap.json`:
- Around line 3-11: Add a short top-of-file comment in
packages/tempo/importmap.json stating that the JSON is the sanctioned browser
import map for this package, that any "./dist/..." entries (e.g.,
"`@magmacomputing/tempo/core`", "…/duration", "…/mutate", "…/plugin", "…/library",
"…/enums") must resolve relative to where this JSON is hosted (typically
alongside the package's dist/), and explicitly note that "`#tempo/license`" is an
internal/private specifier used at runtime; place this comment at the very top
so readers understand the mixed CDN vs local ./dist resolution expectations.
---
Nitpick comments:
In `@packages/tempo/importmap.json`:
- Line 11: The import map exposes a private-style specifier "`#tempo/license`"
which is unconventional for public import maps; update the project's
documentation (e.g., installation guide or package README) to include a brief
note explaining why the Node.js private-specifier pattern (the "`#tempo/license`"
entry in importmap.json) is intentionally used and how consumers should
interpret it so unfamiliar developers are not confused.
In `@packages/tempo/README.md`:
- Line 74: The h4 heading "#### 1. The Global Bundle (Standard Usage)" under the
"Browser & Native Environments" collapsible uses h4s for numbered options and is
being flagged for skipping heading levels; either change this and the sibling
option headings ("#### 2. ...", "#### 3. ...", etc.) to h3 (###) to maintain
proper hierarchical order, or add a linter suppression comment/configuration for
this intentional structure so the static analysis ignores these h4 option
headings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1006c002-58d5-410d-aa2c-52a7dce70e81
📒 Files selected for processing (3)
packages/tempo/README.mdpackages/tempo/doc/installation.mdpackages/tempo/importmap.json
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary by CodeRabbit