-
-
Notifications
You must be signed in to change notification settings - Fork 16
feat: add support for baseline years #81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Thanks for putting this together. I'm not sure how useful this feature is, so let's discuss back on #78 before iterating on the code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just left a few cleanup notes. Overall this looks good.
Do we want to wait to merge #82 before merging this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there, just left a few comments around optimization.
src/rules/require-baseline.js
Outdated
// baseline year | ||
type: "integer", | ||
minimum: 2000, | ||
maximum: new Date().getFullYear() + 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double-checking: is this intentional? The max now would be 2026?
Or did you mean this?
maximum: new Date().getFullYear() + 1, | |
maximum: new Date().getFullYear() - 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes this was intentional (see #81 (comment)) but I just realized the maximum is inclusive. My reasoning was that the current year is technically a valid Baseline year, ie https://web.dev/baseline/2025.
Fixed this to use the current year, but let me know if you'd still prefer YYYY - 1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for all of your work on this.
Prerequisites checklist
What is the purpose of this pull request?
Adds the ability to configure the require-baseline rule to target a Baseline year as an alternative to newly or widely available status
What changes did you make? (Give an overview)
available
configuration property enumBaselineAvailability
class to determine whether a feature is supported based on the configRelated Issues
fixes #78
Is there anything you'd like reviewers to focus on?
For the rule schema, would you prefer to configure the baseline year as a separate optional property, rather than shoehorning it in with
available
?