Check published types in CI #1702
Conversation
|
Warning Rate limit exceeded@alexeyr-ci has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThis pull request introduces a new job step called "Lint package publishing" in the GitHub Actions workflow for linting JavaScript and Ruby. It updates the package configuration by changing the main entry point in Changes
Suggested reviewers
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
.github/workflows/lint-js-and-ruby.yml(1 hunks)package.json(2 hunks)script/convert(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
package.json (1)
Learnt from: Romex91
PR: shakacode/react_on_rails#1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
🪛 GitHub Actions: Lint JS and Ruby
script/convert
[warning] 20-20: Rubocop: [Correctable] Style/RegexpLiteral: Use %r around regular expression.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: rspec-package-tests (newest)
- GitHub Check: build-dummy-app-webpack-test-bundles (newest)
- GitHub Check: rspec-package-tests (oldest)
- GitHub Check: build-dummy-app-webpack-test-bundles (oldest)
🔇 Additional comments (3)
.github/workflows/lint-js-and-ruby.yml (1)
90-95: LGTM! Good addition of type checking for published packages.The new "Lint package publishing" step using
attwis a valuable addition to ensure type correctness in the published package. The comments clearly explain why you're using the Node16 profile and ignoring certain CJS export rules, along with a plan to address this in version 15.0.0.package.json (2)
5-5: Verify if using ReactOnRails.full.js as main entry point is correct.According to previous feedback (PR #1697), the file
node_package/lib/ReactOnRails.full.jsis autogenerated during the build process and should not be present in the repository. This raises questions about whether it's appropriate to set it as the main entry point.Since the
exportsfield already references this file as the default export (line 9), is changing themainfield necessary? Consider whether this might cause issues with bundlers or importers that don't yet support theexportsfield.
17-17: LGTM! Added dependency to support type checking in CI.Adding
@arethetypeswrong/clias a dev dependency supports the new "Lint package publishing" step in the CI workflow, which aligns with the PR's goal of checking published types.
script/convert
Outdated
| # The below packages don't work on the oldest supported Node version and aren't needed there anyway | ||
| gsub_file_content("../package.json", /"knip": "[^"]*",/, "") | ||
| gsub_file_content("../package.json", /"@arethetypeswrong\/cli": "[^"]*",/, "") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the regular expression style to follow Rubocop guidelines.
The regular expression at line 20 should use the %r notation as suggested by the Rubocop warning.
- gsub_file_content("../package.json", /"@arethetypeswrong\/cli": "[^"]*",/, "")
+ gsub_file_content("../package.json", %r{"@arethetypeswrong/cli": "[^"]*",}, "")The purpose of this line is clear - it removes the @arethetypeswrong/cli package during conversion to support older Node versions, as explained in the comment.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # The below packages don't work on the oldest supported Node version and aren't needed there anyway | |
| gsub_file_content("../package.json", /"knip": "[^"]*",/, "") | |
| gsub_file_content("../package.json", /"@arethetypeswrong\/cli": "[^"]*",/, "") | |
| # The below packages don't work on the oldest supported Node version and aren't needed there anyway | |
| gsub_file_content("../package.json", /"knip": "[^"]*",/, "") | |
| gsub_file_content("../package.json", %r{"@arethetypeswrong/cli": "[^"]*",}, "") |
🧰 Tools
🪛 GitHub Actions: Lint JS and Ruby
[warning] 20-20: Rubocop: [Correctable] Style/RegexpLiteral: Use %r around regular expression.
alexeyr-ci
force-pushed
the
alexeyr/attw-publint
branch
3 times, most recently
from
0b44d4f to
2e3a0eb
Compare
alexeyr-ci
force-pushed
the
alexeyr/attw-publint
branch
from
2e3a0eb to
ff8b4d8
Compare
| # --ignore-rules CJS default export can't be resolved at the moment, | ||
| # revisit in 15.0.0 |
There was a problem hiding this comment.
Actually, maybe we could fix it by using Babel to compile TS instead of TSC. I'm not sure it's worth the effort at the moment.
alexeyr-ci
force-pushed
the
alexeyr/attw-publint
branch
from
8ce19a6 to
83f8820
Compare
| @@ -2,7 +2,7 @@ | |||
| "name": "react-on-rails", | |||
| "version": "14.1.1", | |||
| "description": "react-on-rails JavaScript for react_on_rails Ruby gem", | |||
| "main": "node_package/lib/ReactOnRails.js", | |||
| "main": "node_package/lib/ReactOnRails.full.js", | |||
There was a problem hiding this comment.
We don't have that file anymore.
alexeyr-ci
force-pushed
the
alexeyr/attw-publint
branch
from
83f8820 to
b44f32d
Compare
| @@ -87,6 +87,11 @@ jobs: | |||
| run: yarn start format.listDifferent | |||
| - name: Type-check TypeScript | |||
| run: yarn run type-check | |||
| - name: Lint package publishing | |||
| # --profile because we don't care about node10 | |||
There was a problem hiding this comment.
Resolving ReactOnRails/client under --moduleResolution node is technically possible, but better just to tell people to update their TS config. It isn't a breaking change because nobody needed to resolve it before (and the other change fixes resolution for ReactOnRails itself).
Summary
Add additional checks for
package.jsonissues in CI.Pull Request checklist
- [ ] Add/update test to cover these changes- [ ] Update documentationThis change is
Summary by CodeRabbit