feat: add a target option for the compiled inline script - #30
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe plugin adds an optional esbuild-compatible ChangesCritical script target support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant criticalScriptPlugin
participant esbuild
participant compiledScript
criticalScriptPlugin->>esbuild: Compile critical script with target
esbuild->>compiledScript: Return target-specific JavaScript
criticalScriptPlugin->>compiledScript: Measure compiled script size
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (2)
packages/vite-plugin-critical-script/index.test.ts (1)
66-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the size-limit failure, not only the size difference.
This test never sets
outputSizeLimitand never checks rejection. It cannot detect a regression that checks the limit before target transformation. Use a limit between the untransformed and transformed compiled sizes, assert the modern build resolves, and assert the targeted build rejects with the existing size-limit error.🤖 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/vite-plugin-critical-script/index.test.ts` around lines 66 - 72, The test “applies the target before the size limit is checked” must exercise the size-limit failure, not just compare output lengths. Set outputSizeLimit between the modern and ES2015-transformed sizes, assert the modern runLoad call resolves, and assert the targeted runLoad call rejects with the existing size-limit error.packages/vite-plugin-critical-script/__fixtures__/modern-syntax.ts (1)
1-4: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSeparate syntax coverage from API compatibility.
globalThisandcrypto.randomUUIDare runtime globals and APIs. Thetargetoption transforms syntax such as optional chaining and nullish coalescing, but it does not add API polyfills. Keep this fixture syntax-only, or add a separate runtime/polyfill test.🤖 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/vite-plugin-critical-script/__fixtures__/modern-syntax.ts` around lines 1 - 4, Update the modern-syntax fixture to cover only syntax transformed by the target option, removing runtime-global and API compatibility dependencies such as globalThis, crypto, and randomUUID. If runtime compatibility coverage is needed, place it in a separate polyfill-focused test.
🤖 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/vite-plugin-critical-script/index.ts`:
- Around line 61-63: Update the target guidance in the plugin’s API
documentation to specify an esbuild-compatible resolved browser target rather
than forwarding Vite’s build.target value verbatim, including explaining that
Vite resolves values such as “modules” first. Apply the same clarification
consistently to both the English and Korean API references.
---
Nitpick comments:
In `@packages/vite-plugin-critical-script/__fixtures__/modern-syntax.ts`:
- Around line 1-4: Update the modern-syntax fixture to cover only syntax
transformed by the target option, removing runtime-global and API compatibility
dependencies such as globalThis, crypto, and randomUUID. If runtime
compatibility coverage is needed, place it in a separate polyfill-focused test.
In `@packages/vite-plugin-critical-script/index.test.ts`:
- Around line 66-72: The test “applies the target before the size limit is
checked” must exercise the size-limit failure, not just compare output lengths.
Set outputSizeLimit between the modern and ES2015-transformed sizes, assert the
modern runLoad call resolves, and assert the targeted runLoad call rejects with
the existing size-limit error.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e1c0984-07cf-4c30-b358-3f7deb64e628
📒 Files selected for processing (5)
docs/en/api-reference.mddocs/ko/api-reference.mdpackages/vite-plugin-critical-script/__fixtures__/modern-syntax.tspackages/vite-plugin-critical-script/index.test.tspackages/vite-plugin-critical-script/index.ts
4e46b66 to
38ee94a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/vite-plugin-critical-script/index.test.ts (1)
59-64: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert both modern syntax transformations for browser targets.
The fixture uses both
?.and??, but this test checks only?.. Add the missing assertion so the browser-array path cannot leave nullish coalescing untransformed.Proposed test improvement
expect(output).not.toContain('?.') + expect(output).not.toContain('??') expect(output).toContain('window.__critical')🤖 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/vite-plugin-critical-script/index.test.ts` around lines 59 - 64, Update the “accepts a list of browser targets” test to also assert that the generated output does not contain the nullish coalescing operator (??), alongside the existing optional-chaining assertion. Keep the existing window.__critical assertion unchanged.docs/en/api-reference.md (1)
52-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the American English form
afterward.Replace
afterwardswithafterwardin this API reference.Proposed wording fix
-Compiling down costs bytes, and the result is measured against [`outputSizeLimit`](`#outputsizelimit`) afterwards, so a lower target can push a script over the limit. +Compiling down costs bytes, and the result is measured against [`outputSizeLimit`](`#outputsizelimit`) afterward, so a lower target can push a script over the limit.🤖 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 `@docs/en/api-reference.md` at line 52, Update the wording in the API reference sentence around outputSizeLimit to use the American English form “afterward” instead of “afterwards,” without changing the surrounding meaning.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@docs/en/api-reference.md`:
- Line 52: Update the wording in the API reference sentence around
outputSizeLimit to use the American English form “afterward” instead of
“afterwards,” without changing the surrounding meaning.
In `@packages/vite-plugin-critical-script/index.test.ts`:
- Around line 59-64: Update the “accepts a list of browser targets” test to also
assert that the generated output does not contain the nullish coalescing
operator (??), alongside the existing optional-chaining assertion. Keep the
existing window.__critical assertion unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ecdddb3c-edf6-4f77-8048-5356f9175262
📒 Files selected for processing (4)
docs/en/api-reference.mddocs/ko/api-reference.mdpackages/vite-plugin-critical-script/index.test.tspackages/vite-plugin-critical-script/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/vite-plugin-critical-script/index.ts
- docs/ko/api-reference.md
38ee94a to
0804490
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/en/api-reference.md (1)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify what “transformed code” means.
The
targetoption transforms the critical script with esbuild. Therefore, “cannot rely on any polyfill or transformed code” is ambiguous. State that the script cannot rely on application-level polyfills or transformations applied elsewhere.Proposed wording
- A critical script cannot rely on any polyfill or transformed code. + A critical script cannot rely on application-level polyfills or transformations; set `target` for the critical script itself.🤖 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 `@docs/en/api-reference.md` at line 46, Update the target-option guidance in the API reference to clarify that the critical script cannot rely on application-level polyfills or transformations applied elsewhere, rather than broadly saying it cannot rely on transformed code. Keep the surrounding recommendation about matching supported browsers unchanged.
🤖 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.
Nitpick comments:
In `@docs/en/api-reference.md`:
- Line 46: Update the target-option guidance in the API reference to clarify
that the critical script cannot rely on application-level polyfills or
transformations applied elsewhere, rather than broadly saying it cannot rely on
transformed code. Keep the surrounding recommendation about matching supported
browsers unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d70f7e74-841c-44e2-99aa-8779dffbfbc3
📒 Files selected for processing (4)
docs/en/api-reference.mddocs/ko/api-reference.mdpackages/vite-plugin-critical-script/index.test.tspackages/vite-plugin-critical-script/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/vite-plugin-critical-script/index.ts
- docs/ko/api-reference.md
solo5star
left a comment
solo5star
left a comment
There was a problem hiding this comment.
문서쪽 리뷰 확인 부탁드립니다~ 영문 버전도 동일하게 적용해주시면 감사하겠습니다.
0804490 to
2b2ba86
Compare
|
리뷰 세 건 모두 반영하고 영문에도 동일하게 적용했습니다. 작업 중에 별개 문제를 하나 발견해서 함께 처리했습니다. 문서가 두 벌로 관리되고 있습니다. 그래서 이 PR 이 다만 이 이중 관리는 다음에도 같은 누락을 만들 구조입니다. 동기화 스크립트를 두거나 한쪽을 단일 출처로 삼는 정리가 별도로 필요해 보입니다. 이 PR 범위를 넘어서서 손대지 않았습니다. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/critical-script/src/content/docs/en/api-reference.md`:
- Line 53: Clarify the transformation constraint in both language copies: in
docs/critical-script/src/content/docs/en/api-reference.md lines 53-53, replace
“or transformed code” with wording that specifically refers to transformations
applied elsewhere; in docs/critical-script/src/content/docs/ko/api-reference.md
lines 55-55, revise “변환 코드” to make clear it means externally transformed code,
not output generated by the target setting.
- Line 59: Correct the Vite version mapping in both
docs/critical-script/src/content/docs/en/api-reference.md:59-59 and
docs/critical-script/src/content/docs/ko/api-reference.md:61-61: document Vite
6’s default as “modules” and Vite 7’s default as “baseline-widely-available,”
while preserving the existing correct Vite 5 expansion.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 90262bd4-27bd-4750-9cff-0fef45a53719
📒 Files selected for processing (4)
docs/critical-script/src/content/docs/en/api-reference.mddocs/critical-script/src/content/docs/ko/api-reference.mddocs/en/api-reference.mddocs/ko/api-reference.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/ko/api-reference.md
- docs/en/api-reference.md
2b2ba86 to
2683195
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/en/api-reference.md`:
- Line 56: Correct the Vite version mapping at docs/en/api-reference.md:56 and
docs/ko/api-reference.md:56: state that Vite 6 defaults to “modules” and Vite 7
introduced “baseline-widely-available” as the default. Apply the same
documentation correction in both files.
- Line 50: The target setting applies esbuild syntax transformations to the
critical script’s own build but does not provide API polyfills; update the
wording in docs/en/api-reference.md at line 50 and the equivalent text in
docs/ko/api-reference.md at line 50 to state that the critical script must not
depend on transformations or polyfills from the main bundle.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f41e199d-d2bc-4367-ba9b-174243595c2d
📒 Files selected for processing (3)
docs/en/api-reference.mddocs/ko/api-reference.mdpackages/vite-plugin-critical-script/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/vite-plugin-critical-script/index.ts
esbuild compiled the inline script with its own default of esnext and there was no way to change it. That default is a poor fit for what this plugin produces, because a critical script runs with no safety net: whatever transpiles and polyfills the rest of the application has not been downloaded yet, so a single unsupported token is a parse error that makes the browser discard the whole script. Nothing surfaces on the page, and the prefetch or bridge call the script existed for simply never happens. Pass `target` straight through to esbuild so a build that supports older browsers can compile the inline script down to the same level as the rest of the application. The default is unchanged. Leaving the option unset keeps esbuild's esnext, so existing output is byte for byte what it was. Signed-off-by: jaehuiui <ljh9081@kaist.ac.kr>
2683195 to
379e670
Compare
2 participants
배경
인라인 스크립트는 esbuild 기본값인
esnext로 컴파일되고 있었고, 이를 바꿀 방법이 없었습니다.크리티컬 스크립트는 별도의 폴리필이나 변환 코드에 의존할 수 없는 위치에서 실행됩니다. 지원하지 않는 문법이나 토큰이 하나라도 있으면 파싱 오류로 스크립트 전체가 실행되지 않고, 화면에는 아무 오류도 드러나지 않습니다. 그 스크립트가 존재하던 이유인 프리페치나 브릿지 호출이 조용히 사라집니다.
추가한 것
criticalScriptPlugin에target옵션을 추가하고 esbuild 로 그대로 전달합니다.타입은
esbuild.BuildOptions['target']을 그대로 사용합니다. esbuild 가 받는 형식이 늘어나도 별도 수정이 필요 없습니다.build.target등으로 구형 브라우저를 지원하는 애플리케이션이라면 여기에도 같은 값을 넘겨서 버전을 통일할 수 있습니다.기본값은 바꾸지 않았습니다
옵션을 지정하지 않으면 esbuild 기본값인
esnext가 그대로 적용됩니다. 기존 사용자의 출력은 바이트 단위로 동일합니다.대신 이 값을 명시적으로 지정하는 편이 좋은 이유를 JSDoc 과 API 레퍼런스에 남겼습니다.
테스트
__fixtures__/modern-syntax.ts를 추가하고 네 가지를 검증합니다.?.와??가 출력에 남습니다target: 'es2015'를 주면 두 문법이 모두 사라집니다outputSizeLimit검사 대상이 됩니다마지막 항목이 중요합니다. 타겟을 낮추면 출력이 커지므로 기존 상한을 넘길 수 있고, 그 순서 관계를 테스트가 고정합니다.
문서
docs/en/api-reference.md와docs/ko/api-reference.md의 옵션 표에target을 추가하고 각각 설명 절을 붙였습니다.두 문서는
main상태에서도 prettier 검사를 통과하지 않는 기존 서식 드리프트가 있습니다. 이번에 수정한 부분 외에는 건드리지 않았습니다.검증
CI 와 같은 순서로
lint,typecheck,test,build가 모두 통과합니다. 테스트는 기존 4개에 신규 4개를 더해 8개 전부 통과합니다.Summary by CodeRabbit
New Features
targetoption for critical inline scripts.esnextoutput while allowing transpilation for older browsers.Documentation
Bug Fixes