Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Dynamically validate & set engines.node in package.json on firebase init functions #3894

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Dynamically validate & set engines.node in package.json on firebase init functions #3894

wants to merge 5 commits into from

Conversation

oliver-pham
Copy link

@oliver-pham oliver-pham commented Nov 9, 2021 •
edited
Loading

Description

Fix #3407

This PR should help the engines.node attribute in package.json file dynamically set to the current Node.js version on a dev's machine. If their Node.js version is not supported by Google Cloud Functions, an error will be logged.

For example, if the Node.js version on a dev's machine is 12. After running firebase init functions, the generated package.json file should contain thess lines:

"engines": { 
  "node": "12"
}

If the Node.js version is no longer supported in Google Cloud Functions, a warning will be displayed after firebase init functions:

⚠ Node 8 is no longer supported in Google Cloud Functions.
⚠ See https://firebase.google.com/docs/functions/manage-functions for more details

Scenarios Tested

  • Automatically detect, validate and set Node engine version for package.json with ESLint for Typescript & Javascript
  • Automatically detect, validate and set Node engine version for package.json without ESLint for Typescript & Javascript

@oliver-pham
Added support for autodetection of engines.node version
a1e9632 
When running `firebase init functions`, the generated `package.json`
file will have the value of `engines.node` attribute dynamically set to
the Node.js version on dev's machine.
@google-cla google-cla bot added the cla: yes Manual indication that this has passed CLA. label Nov 9, 2021
@joehan joehan requested review from taeold and inlined November 12, 2021 19:24
taeold
taeold reviewed Nov 12, 2021
View reviewed changes
Copy link
Contributor

@taeold taeold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start! Thanks for your contribution.

Can we also do the same trick for JS template in addition to typescript template?

src/init/features/functions/typescript.js
@@ -6,6 +6,7 @@ var path = require("path");

var npmDependencies = require("./npm-dependencies");
var { prompt } = require("../../../prompt");
var utils = require("../../../utils");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are only using getNodeVersionString, I think it make sense to do something like

var { getNodeVersionString}  = require("../../../utils");

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intend to use logWarning() from utils to warn users of deprecated Node versions (like this code below):

firebase-tools/src/accountImporter.js

Lines 163 to 166 in f413eb9

if (!options.hashAlgo) {
utils.logWarning("No hash algorithm specified. Password users cannot be imported.");
return { valid: true };
}

In that case, should I keep it that way?

src/init/features/functions/typescript.js
@@ -64,3 +65,11 @@ module.exports = function (setup, config) {
return npmDependencies.askInstallDependencies(setup.functions, config);
});
};

function getPackage(useLint) {
const nodeEngineVersion = utils.getNodeVersionString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When do we ever check that the user's node version is one supported by Google Cloud Functions?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taeold I was thinking of using isDeprecatedRuntime() to check for deprecated node version and utils.logWarning() to display the warning message. Something like this:

if (isDeprecatedRuntime(`nodejs${nodeEngineVersion}`)) {
    utils.logWarning("Your Node version is not supported in Google Cloud Functions.");
}

If it's good enough, I'll go ahead and update the code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼 Tiny bug I'd watch out for is that something like nodejs13, which is not supported by Google Cloud Functions, may pass the isDeprecatedRuntime() check. You might to throw in isValidRuntime check somewhere in the code.

@oliver-pham
Copy link
Author

This is a great start! Thanks for your contribution.

Can we also do the same trick for JS template in addition to typescript template?

Sure! I'll do the same thing for JS template after I'm done with the Typescript one.

@oliver-pham oliver-pham marked this pull request as ready for review November 16, 2021 03:48
@oliver-pham oliver-pham requested a review from taeold November 16, 2021 03:52
@oliver-pham
Copy link
Author

Hi @taeold, I just wanna circle back to this PR and see if it's still on your radar. Let me know if minor changes need to be made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inlined inlined Awaiting requested review from inlined

@taeold taeold Awaiting requested review from taeold

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
cla: yes Manual indication that this has passed CLA.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

firebase init functions to set package.json's engines.node version to the dev's machine
2 participants
@oliver-pham @taeold