feat(treeshake-checker): treeshake checker package#623
feat(treeshake-checker): treeshake checker package#623
Conversation
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
ryanbas21
changed the title
|
View your CI Pipeline Execution ↗ for commit 8a8cefe
☁️ Nx Cloud last updated this comment at |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| @@ -0,0 +1,97 @@ | |||
| // tools/treeshake-check/src/lib/treeshake-check.test.ts | |||
| import { describe, expect, it, layer } from '@effect/vitest'; | |||
…wing and error path Replace if-guard narrowing with vitest assert() so type narrowing doubles as a hard assertion. Add hints assertions on the clean-package test. Replace the duplicate clean-package test with a BundleFailed error-path test backed by a bad-syntax fixture.
Replaces the regex heuristic for top-level call detection with an acorn AST walk so import statements and variable-assigned calls no longer false-positive as UnannotatedCall. Regex remains as fallback for unparseable code.
Adds resolveEntry() which reads the exports field (subpath map, flat conditions, or bare string) before falling back to module/main. Conditions priority: import > module > default. require-only exports correctly return MissingEntryPoint rather than silently using a CJS path.
ryanbas21
force-pushed
the
treeshake-check-main
branch
from
3c2fdea to
99ffed1
Compare
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We add the missing dist/index.js fixture files for the enum-only and mixed packages, along with a __fixtures__/.gitignore override to allow tracking them despite the root **/dist ignore rule. Without these files, rollup silently treated the unresolvable paths as external imports, producing empty chunk.modules and incorrectly returning FullyTreeshakeable instead of HasSideEffects.
Warning
❌ We could not verify this fix.
Suggested Fix changes
diff --git a/tools/treeshake-check/src/__fixtures__/.gitignore b/tools/treeshake-check/src/__fixtures__/.gitignore
new file mode 100644
index 0000000..afe1555
--- /dev/null
+++ b/tools/treeshake-check/src/__fixtures__/.gitignore
@@ -0,0 +1,2 @@
+!dist/
+!dist/**
diff --git a/tools/treeshake-check/src/__fixtures__/enum-only/dist/index.js b/tools/treeshake-check/src/__fixtures__/enum-only/dist/index.js
new file mode 100644
index 0000000..445195c
--- /dev/null
+++ b/tools/treeshake-check/src/__fixtures__/enum-only/dist/index.js
@@ -0,0 +1,7 @@
+var Status;
+(function (Status) {
+ Status[Status["Active"] = 0] = "Active";
+ Status[Status["Inactive"] = 1] = "Inactive";
+})(Status || (Status = {}));
+
+export { Status };
diff --git a/tools/treeshake-check/src/__fixtures__/mixed/dist/index.js b/tools/treeshake-check/src/__fixtures__/mixed/dist/index.js
new file mode 100644
index 0000000..02e230d
--- /dev/null
+++ b/tools/treeshake-check/src/__fixtures__/mixed/dist/index.js
@@ -0,0 +1,11 @@
+var Status;
+(function (Status) {
+ Status[Status["Active"] = 0] = "Active";
+})(Status || (Status = {}));
+
+function Widget() {}
+Widget.prototype.render = function () {
+ return 'widget';
+};
+
+export { Status, Widget };
Or Apply changes locally with:
npx nx-cloud apply-locally wTLF-9EdP
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
The root .gitignore's **/dist rule was silently excluding the fixture dist files from git. In CI these files didn't exist, so rollup treated the entry as external and returned FullyTreeshakeable for enum-only and mixed, failing their HasSideEffects assertions. - Adds a fixtures-scoped .gitignore negation (consistent with how bad-syntax/dist is already tracked) - Commits the three missing dist/index.js fixture files - Excludes __fixtures__/**/dist from the eslint plugin scan - Adds the __fixtures__ pattern to nx.json eslint plugin exclude list - Migrates integration tests from `live` to `layer + it.scoped` to align with the rest of the test suite and fix type compatibility
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
|
Deployed a932535 to https://ForgeRock.github.io/ping-javascript-sdk/pr-623/a9325357a0ed3f593f18c0e81ca90936b0652183 branch gh-pages in ForgeRock/ping-javascript-sdk |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (17.61%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #623 +/- ##
===========================================
- Coverage 70.90% 17.61% -53.29%
===========================================
Files 53 154 +101
Lines 2021 24243 +22222
Branches 377 1159 +782
===========================================
+ Hits 1433 4271 +2838
- Misses 588 19972 +19384 🚀 New features to boost your workflow:
|
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/device-client - 0.0 KB (-10.0 KB, -100.0%) 📊 Minor Changes📈 @forgerock/device-client - 10.0 KB (+0.0 KB) ➖ No Changes➖ @forgerock/davinci-client - 48.9 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
Fixture JS files are hand-authored test data, not build output. Moving them from dist/index.js to index.js avoids conflicts with the root **/dist gitignore rule, which was silently excluding them from git. In CI the files were missing, causing rollup to treat them as external and return FullyTreeshakeable for enum-only and mixed, failing the HasSideEffects assertions. Also migrates integration tests from `live` to `layer + it.scoped` to align with the rest of the test suite.
ryanbas21
force-pushed
the
treeshake-check-main
branch
from
19b6957 to
8a8cefe
Compare
2 participants
JIRA Ticket
N/A
Description
I don't know the company rules around packages that I write on the computer computers, so i'll ask if we want it here. It's what I demo'd on Friday that shows how to check if a package is treeshakeable, and can aid you in what is causing it to not be treeshakeable.
There's a few optional things I could add to it, but i'll leave it here for now.