-
Notifications
You must be signed in to change notification settings - Fork 897
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
Add node engines fields specifying a minimum of 18 #8457
Conversation
|
Size Report 1Affected ProductsNo changes between base commit (5594ebc) and merge commit (503e462).Test Logs |
Size Analysis Report 1Affected ProductsNo changes between base commit (5594ebc) and merge commit (503e462).Test Logs |
packages/analytics/package.json
Outdated
@@ -71,5 +71,8 @@ | |||
".ts" | |||
], | |||
"reportDir": "./coverage/node" | |||
}, | |||
"engines": { | |||
"node": ">=18.0.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.
The spreadsheet notes that analytics and some of the types packages aren't supported in node. I had assumed that we wouldn't see a node engines entry in those packages' package.json files..?
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.
Yep, looks like the script didn't work, I'll fix it.
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 updated the PR and also made a gist of the array in my script telling the script which packages to add this to, to make it easier for you to audit than having to go through all the files in this PR https://gist.github.com/hsubox76/e58e8a45d1cb04af7ebb31ebff07365d Let me know if any of them look incorrect.
4e98861
to
40481b5
Compare
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 from firestore
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.
Approval for auth
Update all appropriate
package.json
files in the repo to reflect a minimum Nodeengines
version of 18.Exceptions:
auth/cordova
, orfirebase/compat/storage
(the actual published package is@firebase/storage-compat
).Notes:
component
,logger
, andutil
should be platform agnostic, but this enables us to introduce any new APIs (e.g., new cryptography API) that old versions of Node don't support, if we need to in the futureSee https://docs.google.com/spreadsheets/d/1TpQleZQiw1AW_E_CQE12mfW0cRNLvwIB03hDsfop7f8/edit?gid=0&resourcekey=0-k7oyij2BtGpazx4_2cexuA#gid=0 (internal link) for a breakdown of exactly which packages were included or excluded and some notes as to why.