-
Notifications
You must be signed in to change notification settings - Fork 925
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
Updates prettier and eslint to support new Typescript features #2045
Conversation
`Google API requested!\n - URL: "${ | ||
systemLog.data.href | ||
}"\n - Be careful, this may be a production service.` | ||
`Google API requested!\n - URL: "${systemLog.data.href}"\n - Be careful, this may be a production service.` |
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.
Did this change our max line length? Or is there a special allowance for long strings. Fwiw I like this change just wondering why it happened.
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 think prettier used to let you have longer strings but when template strings came along they made the decision to break within the ${}
blocks. Evidently that's turned around.
Personally (don't change this right now because it's fine), I would rather see EmulatorLogger.log
functions for each line rather than injecting \n
into the strings. It's much harder to reason about when logs aren't on one line (maybe this shouldn't have newlines at all?)
Changes LGTM but I'd like @bkendall to approve 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.
Nothing to actually change on top of this. Thanks for taking a look!
"firebase": "^7.1.0", | ||
"firebase-admin": "^8.6.1", | ||
"firebase-functions": "^3.1.0", | ||
"mocha": "^5.0.5", | ||
"nock": "^9.3.3", | ||
"nyc": "^14.0.0", | ||
"prettier": "1.14.3", | ||
"prettier": "^1.19.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.
Of note: prettier
used to not have a ^
there (so it was pinned to a version), but package-lock.json
should do functionally the same thing, so I don't think it's worth removing.
`Google API requested!\n - URL: "${ | ||
systemLog.data.href | ||
}"\n - Be careful, this may be a production service.` | ||
`Google API requested!\n - URL: "${systemLog.data.href}"\n - Be careful, this may be a production service.` |
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 think prettier used to let you have longer strings but when template strings came along they made the decision to break within the ${}
blocks. Evidently that's turned around.
Personally (don't change this right now because it's fine), I would rather see EmulatorLogger.log
functions for each line rather than injecting \n
into the strings. It's much harder to reason about when logs aren't on one line (maybe this shouldn't have newlines at all?)
** and to display scheduled functions in the Firebase console | ||
** If you change this pattern, Firebase console will stop displaying schedule descriptions | ||
** and schedules created under the old pattern will no longer be cleaned up correctly | ||
*/ |
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.
These blocks shouldn't have double asterisks, but not worth changing right now.
Updated to the latest versions of prettier, eslint, and the various eslint plugins we use, and then ran
npm run format
to apply the new rules.This should let us do fun things like optional?.chaining!