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

fix(edge-functions): add deno.json fallback #2984

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

avallete
Copy link
Member

What kind of change does this PR introduce?

Closes: #2983

What is the current behavior?

Please link any relevant issues here.

What is the new behavior?

Feel free to include screenshots if it includes visual changes.

Additional context

Add any other context or screenshots.

@avallete avallete requested a review from a team as a code owner December 19, 2024 03:47
@coveralls
Copy link

coveralls commented Dec 19, 2024

Pull Request Test Coverage Report for Build 12510084008

Details

  • 36 of 47 (76.6%) changed or added relevant lines in 3 files are covered.
  • 7 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.009%) to 59.735%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/functions/deploy/deploy.go 20 21 95.24%
internal/functions/new/new.go 16 19 84.21%
internal/utils/misc.go 0 7 0.0%
Files with Coverage Reduction New Missed Lines %
internal/functions/new/new.go 2 87.18%
internal/gen/keys/keys.go 5 12.9%
Totals Coverage Status
Change from base Build 12446786857: -0.009%
Covered Lines: 7667
Relevant Lines: 12835

💛 - Coveralls

@avallete
Copy link
Member Author

This would fix the issue, but I wonder if we should rather move toward a more "isolation" approach with something like this in the docs for the edge functions:

**Note:** Each Edge Function should be treated as an independent project with its own `deno.json` or `import_map.json` file. Avoid placing shared configurations at the root of the `functions` directory, as this can lead to conflicts. This ensures functions remain self-contained and portable.

Could also mention the .npmrc as mentioned here: #2979

I'm happy to close the PR and go toward this approach if we choose it instead.

jgoux
jgoux previously approved these changes Dec 19, 2024
@sweatybridge
Copy link
Contributor

Two suggestions.

  1. We should handle both .json and .jsonc like in per function config parsing.
  2. I agree we want to encourage isolation for deployed functions. The default import map was only added for convenience for local dev and serving.

I'd prefer updating the docs to mention the suggested path for deno.json for deployment so it's less confusing for new users.

@avallete
Copy link
Member Author

avallete commented Dec 20, 2024

Two suggestions.

  1. We should handle both .json and .jsonc like in per function config parsing.
  2. I agree we want to encourage isolation for deployed functions. The default import map was only added for convenience for local dev and serving.

I'd prefer updating the docs to mention the suggested path for deno.json for deployment so it's less confusing for new users.

Good spot for the jsonc.

I think the issue with the "convenience shortcut" is that it create the expectation that things will work the same way between all the ways you can define your deps. And even if we mention things in the docs, it's not the best for discovery.

Here is what I would like to propose:

  1. Support the "global deno.json/deno.jsonc" for consistency with the import_map.json shortcut behavior.
  2. Mention in the docs that each function should be isolated, etc...
  3. Add a warning for every function that use the fallback on the cli side. So it stand out for the user that this is not a suitable ways to do things and should only be used for development, redirect to the docs (prepare to remove this completely for the next major as a breaking change thing ? We could provide a shellscript'ish way to easily do the migration by copying the global into the subfolders)
  4. Maybe a good way to nudge the users in the right direction is that when we do functions new, on top of the index.ts also create a deno.json and .npmrc files in the created folders ?

I've reworked things toward this and refactored a bit how we handle the "default path", and created a PR for the documentation here: supabase/supabase#31264

WDYT ? @sweatybridge @jgoux

}
}
if noVerifyJWT != nil {
function.VerifyJWT = cast.Ptr(!*noVerifyJWT)
}
functionConfig[name] = function
}
// Check validity of ImportMap paths
functionsWithFallback := []string{}
if fallbacksPath, err := utils.GetImportsFilePath(utils.FunctionsDir, fsys); err == nil {
Copy link
Contributor

@sweatybridge sweatybridge Dec 20, 2024

Choose a reason for hiding this comment

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

If we really want to support fallback import map for deno.json, then this should be moved to pkg/config so it also works for branching.

Each function config's import_map field will point to the fully resolved path after considering all possible fallbacks.

Copy link
Member Author

@avallete avallete Dec 20, 2024

Choose a reason for hiding this comment

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

question

It seems like we didn't supported this fallback before ? I don't know how that was handled with branching I don't see the code logic for the import_map.json fallback in the pkg/config 🤔

I have two question about this:

  1. Won't it have side effects with the diffing of the config in such case ?
  2. I think we can't import utils within pkg/config without causing a cycling deps issue. What would be the best approach here ?

@jgoux
Copy link
Contributor

jgoux commented Dec 20, 2024

+1 with @sweatybridge on only updating the docs and explaining that one deno.json(c) per function is the best practice, and maybe add a deprecated mention on the import_map.json file in functions.

@sweatybridge sweatybridge dismissed jgoux’s stale review December 30, 2024 03:35

dismissing to prefer docs update #2984 (comment)

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.

Supabase functions prioritize 'import_map.json' over 'deno.json' for import maps.
4 participants