Skip to content

fix: outside import with relative module graph#3417

Merged
sweatybridge merged 14 commits into
supabase:developfrom
kallebysantos:fix-outside-relative-import
Apr 12, 2025
Merged

fix: outside import with relative module graph#3417
sweatybridge merged 14 commits into
supabase:developfrom
kallebysantos:fix-outside-relative-import

Conversation

@kallebysantos

Copy link
Copy Markdown
Member

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

When using outside import for local development, the bundler does not include relative imports in the Module graph,
Like the following example

Error
worker boot error: failed to create the graph: Module not found "file:///Users/kallebysantos/projects/testing/supa/outside-imports/my-lib/add.ts".
    at file:///Users/kallebysantos/projects/testing/supa/outside-imports/my-lib/index.ts:1:21
worker boot error: failed to create the graph: Module not found "file:///Users/kallebysantos/projects/testing/supa/outside-imports/my-lib/add.ts".
    at file:///Users/kallebysantos/projects/testing/supa/outside-imports/my-lib/index.ts:1:21
InvalidWorkerCreation: worker boot error: failed to create the graph: Module not found "file:///Users/kallebysantos/projects/testing/supa/outside-imports/my-lib/add.ts".
    at file:///Users/kallebysantos/projects/testing/supa/outside-imports/my-lib/index.ts:1:21
    at async UserWorker.create (ext:sb_user_workers/user_workers.js:139:15)
    at async Object.handler (file:///root/index.ts:157:22)
    at async respond (ext:sb_core_main_js/js/http.js:197:14) {
  name: "InvalidWorkerCreation"
}

What is the new behavior?

This PR applies the same import resolution as when deploying from --use-api. So it ensures that all files are bundled for local development.

Fixed

image

@kallebysantos kallebysantos requested a review from a team as a code owner April 10, 2025 19:16
@kallebysantos kallebysantos changed the title fix: outside relative import graph fix: outside import with relative module graph Apr 10, 2025
@jumski

jumski commented Apr 10, 2025

Copy link
Copy Markdown

Yes please! This makes the outside imports usable 🎉

@laktek laktek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! This applies to local development as in functions serve right?

@sweatybridge and CLI team will review & merge it.

Comment thread internal/functions/deploy/bundle.go Outdated
@kallebysantos kallebysantos force-pushed the fix-outside-relative-import branch from 77b2976 to 861580d Compare April 10, 2025 20:08
@kallebysantos

Copy link
Copy Markdown
Member Author

This applies to local development as in functions serve right?

Yes, it will bind the missing files to the docker container

Comment thread pkg/function/deploy.go Outdated
Comment thread pkg/function/deploy.go
It applies the import map walking to resolve the Module Graph
Moving `ImportMap` struct to `functions/pkg` and importing it inside
internals
@kallebysantos kallebysantos force-pushed the fix-outside-relative-import branch from 0ace083 to 7f0ae8f Compare April 11, 2025 08:43
Comment thread pkg/function/deploy.go Outdated
Comment thread pkg/function/deploy.go Outdated
Comment thread internal/functions/deploy/bundle.go Outdated
Comment thread internal/functions/deploy/bundle.go Outdated
Comment thread internal/utils/deno.go Outdated
- we don't need to tee here, just copy from file to the writer will do.
- using `afero.NewIOFS(fsys)` to cast to `io.FS`
- Convert `srcPath` inside `addFile`, since docker mount always expect
an absolute path
Comment thread internal/functions/deploy/bundle.go Outdated
}
// Resolving all Import Graph
addModule := func(unixPath string, w io.Writer) error {
hostPath := filepath.FromSlash(unixPath)

@kallebysantos kallebysantos Apr 11, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hey @sweatybridge I think u did revert the absolute path commitc6da3a7. Was that intentional?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yea.. actually the implementation of walkImportPaths assumed inputs are relative paths only, for a few reasons.

  1. api upload doesn't support absolute path as the name of form upload
  2. we can't get back the absolute path on windows without assuming the drive letter is always the same as cwd

so there are still a bit of touch up / refactoring i need to do after this

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I see, well from my tests except by the supabase/function/slug/index.ts file all others was already returned as fullpath from the walkImportPaths method.
You can see it by printing out the unixPath variable

Comment thread internal/functions/deploy/bundle.go
@sweatybridge sweatybridge merged commit 3dfb4a1 into supabase:develop Apr 12, 2025
@kallebysantos kallebysantos deleted the fix-outside-relative-import branch April 13, 2025 10:42
@github-actions github-actions Bot mentioned this pull request Apr 15, 2025
@wyozi

wyozi commented Apr 15, 2025

Copy link
Copy Markdown

@kallebysantos does this require some special configuration or flags to supabase serve to work? I tried to use code based on your repo and supabase/cli v2.22.1 to get local relative imports to work but no dice

e: nvm, I had file: prefixes in my import maps, which do not work with this

@kallebysantos

Copy link
Copy Markdown
Member Author

Hey @wyozi, sorry about that confusion 😅
I used the same repo to test other incoming feature #3430, that's why the current code doesn't works with v2.22.1.
I'll patch fix it, but the error is because v2.22.1 still don't support the importMap so the deno.json file is not resolving any imports.

Working example
// supabase/functions/[slug]/deno.json
{
   "imports": {
    "@mylib": "../../../my-lib/index.ts"
  }
}

@wyozi

wyozi commented Apr 15, 2025

Copy link
Copy Markdown

@kallebysantos ah yeah, good stuff. I think that feature actually already is in 2.22.1, which is why just removing the file: prefix seems to have solved the problem

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.

5 participants