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.

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 functionality to omit scripts section in package.json #23

Closed
wants to merge 6 commits into from

Conversation

jieey1140
Copy link
Contributor

Firebase Functions relies on the build script from package.json.
However, Cloud Build only recognizes npm, not pnpm, leading to issues with
commands like pnpm nest build. To resolve this, the script section from package.json
is removed, as there is no need to rerun the script commands when the file, which
should already be built and ready for deployment, is being deployed.

Ensure smooth deployments in a pnpm environment by utilizing the omitScripts option to prevent issues during deployment.

============
I am submitting this pull request due to the aforementioned issue. Currently, I am using a separately crafted "remove package script," but integrating it into isolate would be splendid as it would not only streamline the build step during a Firebase deploy but also add an additional feature to isolate!

Please consider the above matter! 😄

Copy link
Owner

@0x80 0x80 left a comment

Choose a reason for hiding this comment

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

I initially had the scripts excluded but then someone requested to include them and I couldn't think of a scenario where you want to omit them, but I'm fine having this as an option. However, your PR adds a lot of code I would prefer to use the configuration in the location that this writing the manifest file, instead of mutating it in a separate process.

Here is the commit that I used to no longer omit the scripts any longer. I think you can apply the configuration there:

6ded7a8

Also, I wonder if you really need this. Can't you simply replace your command pnpm nest build with nest build?

I think it is uncommon to have your package manager as part of the scripts for executing stuff. The binary executable "nest" will be found in node_modules when you use it as part of a script in the manifest. There should be no reason to prefix it with pnpm AFAIK

@jieey1140
Copy link
Contributor Author

@0x80

We execute scripts with this naming convention:

"build": "silvia-script functions:build",

Within this, it builds the necessary monorepo modules. When the script module is outside, it cannot read the command inside without prefixing it with pnpm, hence we inserted pnpm.

Also, in our CI pipeline, we build using nest and execute firebase deploy.

We've noticed that when there’s a build in the scripts section, it builds upon the already built files, elongating the cloud build time.

Thus, our approach has been to exclude the script part and deploy.

I’ve checked the commit content that used to be there and has since disappeared! I’m going to close this PR.

Thank you for letting me know!

@jieey1140 jieey1140 closed this Oct 14, 2023
@0x80
Copy link
Owner

0x80 commented Oct 14, 2023

@jieey1140 I forgot why I got the request to include the scripts but someone needed it. Maybe as a compromise, we can only omit the "scripts.build" and leave the other scripts in tact :)

I'll look up what the issue was with omitting scripts...

@0x80
Copy link
Owner

0x80 commented Dec 18, 2023

@jieey1140 There are now configuration options to pick or omit specific scripts from the output manifest. Also, all package managers now output correct lockfiles and you can choose to force output in NPM format for deployment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants