-
Notifications
You must be signed in to change notification settings - Fork 984
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
WIP: major eslint updates (with eslint-config-standard@next) for evaluation only #390
Conversation
Codecov Report
@@ Coverage Diff @@
## master #390 +/- ##
=======================================
Coverage 65.62% 65.62%
=======================================
Files 14 14
Lines 1702 1702
Branches 286 286
=======================================
Hits 1117 1117
Misses 585 585 Continue to review full report at Codecov.
|
"eslint-plugin-standard": "^3.0.1", | ||
"eslint": "^5.2.0", | ||
"eslint-config-semistandard": "^12.0.1", | ||
"eslint-config-standard": "^12.0.0-alpha.0", |
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.
Is there a reason that this one has to be on @next
?
Could it be eslint-config-standard@^11.0.0
as seen in the other evaluation PR #389 while the rest were updated?
I don't see any problems with updating all of them. I prefer this PR. (+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.
eslint-config-standard@next was needed to resolve an ugly warning message from eslint@5, details are now in the description
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.
FYI ^12.0.0-alpha.0
means that we would use the -alpha version until the publish a newer version, such as -beta, or finally publish 12.0.0 version
To clarify the general question: I would generally favor upgrading to "latest" version of dependencies and devDependencies whenever possible when targeting upcoming major release. The problem is that using eslint@latest would lead to the ugly warning as discussed in the updated description and review comments unless we would use eslint-config-standard@next (eslint-config-standard@^12.0.0-alpha.0). We are basically stuck waiting for eslint-config-standard@12.0.0 to be released as discussed in standard/eslint-config-standard#123. I think we should not introduce "@next" dependency version unless absolutely necessary for some reason. |
Is this still WIP? There are conflicts that need to be resolved. |
Closing in favor #496 which is now merged. |
Platforms affected
iOS
What does this PR do?
WIP for evaluation & discussion only (with an alt-wip branch name):
[ESLINT_LEGACY_OBJECT_REST_SPREAD]
DeprecationWarning (Release 12.0.0 standard/eslint-config-standard#123 (comment)) when using eslint@5, taken from PR WIP - update dev deps (with additional TODO list) #382cannot be backported to 4.5.x since it requires minimum Node.js 6
Alternative using eslint@4, not using alpha eslint-config-standard@next version, is in #389.
What testing has been done on this change?
npm test
items pass on Travis CIChecklist
Reported an issue in the JIRA databaseCommit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.Added automated test coverage as appropriate for this change.