Fix bug where function discovery failed on projects using yarn workspaces. #5215
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Nov 4, 2022
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
taeold
force-pushed
the
dl-cf3-discover-test
branch
from
7c666f6
to
7de7f09
Compare
taeold
force-pushed
the
_dl-cf3-yarn-workspaces
branch
from
b7fd039
to
1822bac
Compare
Codecov ReportBase: 56.27% // Head: 56.28% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## dl-cf3-discover-test #5215 +/- ##
========================================================
+ Coverage 56.27% 56.28% +0.01%
========================================================
Files 309 309
Lines 20838 20828 -10
Branches 4227 4227
========================================================
- Hits 11726 11723 -3
+ Misses 8100 8093 -7
Partials 1012 1012
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
taeold
force-pushed
the
dl-cf3-discover-test
branch
from
7de7f09
to
960f1b4
Compare
taeold
force-pushed
the
_dl-cf3-yarn-workspaces
branch
from
1822bac
to
a2b0f24
Compare
taeold
force-pushed
the
dl-cf3-discover-test
branch
from
960f1b4
to
1d21dee
Compare
taeold
force-pushed
the
_dl-cf3-yarn-workspaces
branch
from
a2b0f24
to
43d41e6
Compare
taeold
force-pushed
the
dl-cf3-discover-test
branch
from
1d21dee
to
ff2647b
Compare
taeold
force-pushed
the
_dl-cf3-yarn-workspaces
branch
from
43d41e6
to
591e866
Compare
taeold
force-pushed
the
dl-cf3-discover-test
branch
from
ff2647b
to
7d28a88
Compare
taeold
force-pushed
the
_dl-cf3-yarn-workspaces
branch
from
591e866
to
deac733
Compare
taeold
force-pushed
the
dl-cf3-discover-test
branch
from
7d28a88
to
dc1163b
Compare
taeold
force-pushed
the
_dl-cf3-yarn-workspaces
branch
from
deac733
to
163e2a3
Compare
taeold
force-pushed
the
dl-cf3-discover-test
branch
from
dc1163b
to
51ddf7e
Compare
taeold
force-pushed
the
_dl-cf3-yarn-workspaces
branch
from
163e2a3
to
af6ff32
Compare
taeold
force-pushed
the
dl-cf3-discover-test
branch
from
51ddf7e
to
95a4810
Compare
taeold
force-pushed
the
_dl-cf3-yarn-workspaces
branch
from
af6ff32
to
b9b8177
Compare
colerogers
approved these changes
Dec 2, 2022
| export function findModuleVersion(name: string, resolvedPath: string): string | undefined { | ||
| let searchPath = path.dirname(resolvedPath); | ||
| // eslint-disable-next-line no-constant-condition | ||
| while (true) { |
There was a problem hiding this comment.
I think search path will always terminate with /, right? Why not use that as the condition instead of doing while (true)?
TheIronDev
approved these changes
Dec 6, 2022
taeold
force-pushed
the
dl-cf3-discover-test
branch
from
95a4810
to
0bdee8d
Compare
taeold
force-pushed
the
_dl-cf3-yarn-workspaces
branch
from
b9b8177
to
a3690a8
Compare
taeold
force-pushed
the
dl-cf3-discover-test
branch
from
0bdee8d
to
cce0388
Compare
taeold
force-pushed
the
_dl-cf3-yarn-workspaces
branch
from
a3690a8
to
250c1fe
Compare
|
Graphite Merge Job Current status: ✅ Merged This pull request was successfully merged as part of a stack. This comment was auto-generated by Graphite. Job Reference: Uhty8UE38l3MKEIRA9gp |
taeold
force-pushed
the
_dl-cf3-yarn-workspaces
branch
from
250c1fe
to
9106087
Compare
taeold
added a commit
that referenced
this pull request
Jan 6, 2023
…orepo setups (#5391) Partially fixes #4952. #5215 made changes to the CLI to detect Functions SDK even in monorepo setups where Functions SDK is hoisted (i.e. Functions SDK dependency is declared in the parent directory but not in the sub-package directory). We should've made corresponding change in how the Functions SDK binary is executed - instead of always looking up the sub-package's `node_modules`, we should be looking at the `node_modules` closest to where the Functions SDK dependency is declared. This setup seems common in scenarios where the developer bundles the functions source using bundlers like vite/webpack/etc. This kind of technique has shown to help [AWS lambda's cold start time](https://aws.amazon.com/blogs/compute/optimizing-node-js-dependencies-in-aws-lambda/), and I think it's something we'd want to explore in Google Cloud Functions too.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Today, the CLI uses the
npm ls firebase-functionscommand to check the version of the firebase functions of the user's functions module. The current implementation doesn't work when using npm/yarn workspaces.We rewrite the implementation to use [
require.resolve] to look up location of the firebase functions package and read the accompanying package.json to look up the version instead. This implementation works with npm/yarn workspaces as demonstrated in the new test cases.Hopefully addresses #5191