-
Notifications
You must be signed in to change notification settings - Fork 209
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
feat(config): load local deno.json file as import map #2824
Conversation
Pull Request Test Coverage Report for Build 11647813901Details
💛 - Coveralls |
pkg/config/config.go
Outdated
// Load functions config | ||
pattern := filepath.Join(builder.FunctionsDir, "*", "index.ts") | ||
paths, err := fs.Glob(fsys, pattern) | ||
if err != nil { | ||
return errors.Errorf("failed to glob function slugs: %w", err) | ||
} | ||
|
||
if c.Functions == nil { | ||
c.Functions = make(FunctionConfig) | ||
} |
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.
Globbing functions this way is very fragile because users often have shared components that they don't want to deploy as functions, for eg. functions/shared/index.ts
. That was one of the reasons we opted to require explicit declaration of functions in config.toml.
Instead, we want to find deno.json
relative to entrypoint
path.
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 took the code from
cli/internal/functions/deploy/deploy.go
Lines 49 to 66 in 4cf189a
func GetFunctionSlugs(fsys afero.Fs) (slugs []string, err error) { | |
pattern := filepath.Join(utils.FunctionsDir, "*", "index.ts") | |
paths, err := afero.Glob(fsys, pattern) | |
if err != nil { | |
return nil, errors.Errorf("failed to glob function slugs: %w", err) | |
} | |
for _, path := range paths { | |
slug := filepath.Base(filepath.Dir(path)) | |
if utils.FuncSlugPattern.MatchString(slug) { | |
slugs = append(slugs, slug) | |
} | |
} | |
// Add all function slugs declared in config file | |
for slug := range utils.Config.Functions { | |
slugs = append(slugs, slug) | |
} | |
return slugs, nil | |
} |
Should we also change this part then?
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.
No need to update deploy for the time being. We left it for backwards compatibility with those who use setup-cli GitHub action.
Users only need to declare if they want to deploy to branches using our new integration.
3559079
to
0a4fe8d
Compare
0a4fe8d
to
7b4509d
Compare
As per the additional context in the description can a global deno.json now be used instead of an import_map.json with this change? |
What kind of change does this PR introduce?
feature
What is the current behavior?
Currently each edge function shares a global
import_map.json
by default.To use one particular import map per function the user has to define it manually in
config.toml
What is the new behavior?
We now automatically load
deno.json
anddeno.jsonc
under thesupabase/functions/<slug>
directory.Each function should be treated as a different project with its own set of dependencies.
Additional context
import_map.json
disappeared from Deno's doc since as of v1.19.2: https://deno.land/x/[email protected]/npm_nodejs/import_maps.md?sourceImports should now be declared as part of deno's config file
deno.json
.We might want to deprecate
import_map.json
, especially when we will ship deno v2 support.