More precise core-js version #556
Conversation
WalkthroughThis pull request updates the way the Changes
Sequence Diagram(s)sequenceDiagram
participant BP as Babel Preset
participant CSV as coreJsVersion
participant PFV as packageFullVersion
BP->>CSV: Request core-js version
CSV->>PFV: Retrieve package version from package.json
alt Package exists
PFV-->>CSV: Return full version string
CSV-->>BP: Return computed major.minor version
else MODULE_NOT_FOUND Error
PFV-->>CSV: Throw error
CSV-->>BP: Catch error, return default "3.8"
end
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
🪧 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 (
|
alexeyr-ci2
force-pushed
the
alexeyr/core-js-version
branch
from
b8ad4b6 to
ea3289d
Compare
![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 (1)
package/utils/helpers.js (1)
44-51: Consider documenting the version extraction differences.There's an inconsistency between how versions are extracted:
coreJsVersionuses regex to extract major.minor (e.g., "3.8")packageMajorVersionextracts only the major version (e.g., "3")While this may be intentional, it could lead to confusion. Consider adding comments to explain the different extraction patterns or standardize them if they should behave similarly.
🧰 Tools
🪛 ESLint
[error] 51-51: Insert
⏎·(prettier/prettier)
🪛 GitHub Check: Linting
[failure] 51-51:
Insert⏎·🪛 GitHub Actions: Node based checks
[error] 51-51: Insert
⏎·prettier/prettier
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)package/babel/preset.js(3 hunks)package/utils/helpers.js(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
19-19: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🪛 ESLint
package/utils/helpers.js
[error] 51-51: Insert ⏎·
(prettier/prettier)
🪛 GitHub Check: Linting
package/utils/helpers.js
[failure] 51-51:
Insert ⏎·
🪛 GitHub Actions: Node based checks
package/utils/helpers.js
[error] 51-51: Insert ⏎· prettier/prettier
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.3, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.2.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.6.1.x)
- GitHub Check: Generator specs (ubuntu-latest, 3.1, gemfiles/Gemfile-rails.7.0.x)
- GitHub Check: Generator specs (ubuntu-latest, 2.7, gemfiles/Gemfile-rails.7.0.x)
🔇 Additional comments (7)
CHANGELOG.md (1)
17-19: LGTM! Good documentation of the change.The addition to the CHANGELOG clearly explains the change from using a fixed
core-jsversion to dynamically taking the current one fromnode_modules. This matches the PR objectives and provides good context for other developers.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
19-19: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
package/babel/preset.js (4)
1-1: Correctly updated imports to include the new functionality.The import has been properly updated to include the
packageFullVersionfunction that's needed for the new functionality.
3-13: Well-implemented version detection with fallback.This
coreJsVersionfunction is well structured with proper error handling. It:
- Tries to get the version using the new
packageFullVersionfunction- Extracts the major.minor version (e.g., "3.8" from "3.8.1")
- Provides a sensible fallback if the module isn't found
- Properly re-throws other errors
The approach gracefully handles the case where
core-jsisn't installed, maintaining compatibility.
36-36: Good implementation of dynamic version detection.Using the
coreJsVersion()function instead of hardcoding "3.8" is a good improvement that makes the configuration more adaptable to the actual installed package version.
25-25: Improved error message formatting.The error message has been simplified by using direct string interpolation instead of JSON.stringify, making it more readable.
package/utils/helpers.js (2)
44-49: Good implementation of full version retrieval.The
packageFullVersionfunction provides a clean way to get the complete version string from a package's package.json file.
59-60: Successfully updated exports.The module exports have been correctly updated to include both the new
packageFullVersionfunction and the existingpackageMajorVersionfunction.
package/utils/helpers.js
Outdated
| } | ||
|
|
||
| const packageMajorVersion = (packageName) => packageFullVersion(packageName).match(/^\d+/)[0] |
There was a problem hiding this comment.
Fix formatting issue with missing newline.
While the implementation of packageMajorVersion is functionally correct, there's a linting error showing that a newline is missing after this line according to the project's coding standards.
Apply this diff to fix the formatting issue:
-const packageMajorVersion = (packageName) => packageFullVersion(packageName).match(/^\d+/)[0]
+const packageMajorVersion = (packageName) => packageFullVersion(packageName).match(/^\d+/)[0]
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 51-51: Insert ⏎·
(prettier/prettier)
🪛 GitHub Check: Linting
[failure] 51-51:
Insert ⏎·
🪛 GitHub Actions: Node based checks
[error] 51-51: Insert ⏎· prettier/prettier
alexeyr-ci2
force-pushed
the
alexeyr/core-js-version
branch
from
ea3289d to
25b4b71
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
17-20: Markdown List Style Consistency:
The new changelog entry on line 19 uses a dash (-) as the list marker. The markdown linting guidelines recommend using an asterisk (*) for unordered lists. To ensure consistency and adherence to the markdown style guide, consider switching to an asterisk.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
19-19: Unordered list style
Expected: asterisk; Actual: dash(MD004, ul-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)package/babel/preset.js(3 hunks)package/utils/helpers.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package/babel/preset.js
- package/utils/helpers.js
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
19-19: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Generator specs (ubuntu-latest, 3.0, gemfiles/Gemfile-rails.6.1.x)
- GitHub Check: test
| `Please specify a valid NODE_ENV or BABEL_ENV environment variable. Valid values are "development", "test", and "production". Instead, received: "${JSON.stringify( | ||
| currentEnv | ||
| )}".` | ||
| `Please specify a valid NODE_ENV or BABEL_ENV environment variable. Valid values are "development", "test", and "production". Instead, received: "${currentEnv}".` |
There was a problem hiding this comment.
why this change? you might just see Object without the old code.
There was a problem hiding this comment.
Does this actually happen? The documentation says
api.env()returns the current envName string.
Summary
Instead of fixed
core-jsversion, take the current one fromnode_modules.Pull Request checklist
[ ] Add/update test to cover these changes[ ] Update documentationSummary by CodeRabbit
Documentation
New Features
Refactor