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

feat : load driver by uri like file:/// http:/// etc #582

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

AntoineMontane
Copy link

solve #581

@AntoineMontane AntoineMontane requested a review from pi0 as a code owner January 25, 2025 17:21
@AntoineMontane AntoineMontane force-pushed the main branch 3 times, most recently from 6099abe to d9bee91 Compare January 26, 2025 14:17
@AntoineMontane AntoineMontane marked this pull request as draft January 26, 2025 14:18
@pi0 pi0 changed the title Feature URI : load driver by uri like file:/// http:/// etc feat : load driver by uri like file:/// http:/// etc Jan 27, 2025
@pi0
Copy link
Member

pi0 commented Jan 27, 2025

This is a nice idea however as it has bundle size implications, I would keep it completely separate.

Can you please as first step, deduce diff Generate a new entry in dist that is an object mapping from URI scheme like "file", to driver object. (there should be no changes in driver src files)

(we also need to introduce a new subpath export like unstorage/full to export anything related to this from)

@AntoineMontane
Copy link
Author

AntoineMontane commented Jan 27, 2025

True, i was so displeased by the bundling issue i rewrote from scratch (i'm always amazed how good dynamic imports look from far away ;-) )
Now the driverFromUri takes a list of imported driver which means the bundling is left to the library consumer. More flexibility, more control, less issues ...

import http from "unstorage/drivers/http"
import memory from "unstorage/drivers/memory"

driverFromUri(uri, [http, memory])

We could still provide the "full bundle" if needed one way or another, or profiles: "node", "browser", "azure", etc.

BTW i saw that mongodb was not declared as peer / optional and presented issue with the full bundle in my consumer project. fs was also an issue client side ... that was the last straw for the dynamic import tentative.

src/drivers/fs-lite.ts Outdated Show resolved Hide resolved
test/drivers/fs.test.ts Outdated Show resolved Hide resolved
@AntoineMontane
Copy link
Author

I think i should go back to the previous approach to export the regexp from each driver module. And keep the new one for the loader. What do you think of this ?

src/loader/all.ts Outdated Show resolved Hide resolved
import { coerceQuery } from "./utils";
import type { Driver, DriverFactory } from "../types";

const RE = /^(?<protocol>[^:?]+)(:(?<base>[^?]*))?(\?(?<query>.*))?$/;
Copy link
Member

Choose a reason for hiding this comment

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

We should use new URL as parser.

Copy link
Author

@AntoineMontane AntoineMontane Jan 29, 2025

Choose a reason for hiding this comment

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

new URL misses the ability to express relative urls:

> new URL('file:abc/def')
URL {
  href: 'file:///abc/def',
  origin: 'null',
  protocol: 'file:',
  username: '',
  password: '',
  host: '',
  hostname: '',
  port: '',
  pathname: '/abc/def',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}
> new URL('file:/abc/def')
URL {
  href: 'file:///abc/def',
  origin: 'null',
  protocol: 'file:',
  username: '',
  password: '',
  host: '',
  hostname: '',
  port: '',
  pathname: '/abc/def',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}
> new URL('http:/abc/def')
URL {
  href: 'http://abc/def',
  origin: 'http://abc',
  protocol: 'http:',
  username: '',
  password: '',
  host: 'abc',
  hostname: 'abc',
  port: '',
  pathname: '/def',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

Copy link
Author

Choose a reason for hiding this comment

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

i have tries with ufo.parseUrl without much success either

Copy link
Author

Choose a reason for hiding this comment

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

url-parse library looks to be ok ; yet, i'm not sure it's a good idea to bring in another url parsing library

describe("loader", () => {
it("load driver", async () => {
const driver = loadFromUri(
'test:abc?string=def&number=1&boolean=true&array=[2,3,4]&object={"h":5,"i":6,"j":"7"}',
Copy link
Member

Choose a reason for hiding this comment

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

For being a valid URL, it should be something like this:

test:/abc?string=def&number=1&boolean=true&array=[2,3,4]&object={"h":5,"i":6,"j":"7"}

Copy link
Author

@AntoineMontane AntoineMontane Jan 29, 2025

Choose a reason for hiding this comment

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

http url is a subset of all url (take data: as an example of url not having the slash). And we want to be able to express relative url not starting with /. Ex with http

http:relative/path => base: "relative/path"

http:/absolute/path => base: "/absolute/path"

http://fqdn/path => base: "http://fqdn/path"

@AntoineMontane AntoineMontane force-pushed the main branch 4 times, most recently from 14699a7 to 5bf8819 Compare January 29, 2025 19:00
@pi0
Copy link
Member

pi0 commented Jan 29, 2025

Unstorage is primarily targeting server runtimes. Supporting relative URLs is really an edge case that possible in browser-only environments.

(and we should have one entry exporting dynamic imports inside it / no extra browser.ts)

@AntoineMontane
Copy link
Author

AntoineMontane commented Jan 30, 2025

Is it wrong to support browser runtimes ? In fact i started my development especially for this cross-environment ability.
Relative url for file: or db+sqlite: is quite possible & not an edge case either

@pi0
Copy link
Member

pi0 commented Jan 30, 2025

js runtime support depends on drivers (if they support broeser they do) — we shouldn’t make an exception entey for browser.

re parsing, it kinda makes sense for some drivers but also incompatible with URL standards. To be honest i really have to locally play with some ideas my self at this point.

Please ping me when your work finished later when i could will try to work on it.

@AntoineMontane
Copy link
Author

AntoineMontane commented Jan 30, 2025

A RFC about the generality of url for reflexion :
https://datatracker.ietf.org/doc/html/rfc1738#section-2.1

I'm almost done for the generic loader ( but the part we are discussing on). I will work on some drivers (s3, ...) that i plan to use

@AntoineMontane
Copy link
Author

Is it possible someway to mark node:fs external so that it doesn't represent an issue at client bundle build ?

@pi0
Copy link
Member

pi0 commented Jan 30, 2025

that's a bundler-specific config people should do.

Also, we need to have exactly syntax below in the generated list:

() => import("unstorage/drivers/name")

this is because when using import(unstorage/drivers/${name}) bundler cannot statically analyze and has to scan all possible patterns.

@AntoineMontane
Copy link
Author

AntoineMontane commented Jan 30, 2025

I've

  • refactored uri to url,
  • renamed protocol to scheme
  • refactored the factoryloader to dynamicaly import with raw string
  • dropped the browser bundle

It would be less deceptive to only include in the all bundle drivers that were verified or reworked to correctly handle the url protocol (+ i had to guess some schemes without completly understanding what it could mean)

@AntoineMontane AntoineMontane force-pushed the main branch 3 times, most recently from 0fc8557 to 5cb7e08 Compare January 30, 2025 11:23
@AntoineMontane
Copy link
Author

AntoineMontane commented Jan 30, 2025

Hey @pi0 Do you have a good advice to handle driver import location in the all bundle ?
I don't want to import from unstorage/drivers/ because it's not available until build and adds a step before testing.
If i import relatively from ../drivers/, it works in test but fails in production as the path is different (../../drivers/)
What was the point to output the build of drivers outside of the ./dist driectory ?

@AntoineMontane AntoineMontane marked this pull request as ready for review February 3, 2025 07:39
Comment on lines +15 to +23
const base =
opts.base.startsWith("//") && opts.scheme
? `${opts.scheme}:${opts.base}`
: opts.base;

const r = (key: string = "") => joinURL(base!, key.replace(/:/g, "/"));

const rBase = (key: string = "") =>
joinURL(opts.base!, (key || "/").replace(/:/g, "/"), ":");
joinURL(base!, (key || "/").replace(/:/g, "/"), ":");
Copy link
Member

Choose a reason for hiding this comment

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

Please split this into a new PR with a clear description of new behavior

@@ -36,7 +48,7 @@ export interface S3DriverOptions {
/**
* The name of the bucket.
*/
bucket: string;
bucket?: string;
Copy link
Member

Choose a reason for hiding this comment

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

endpoint should not be deprecated because of a new feature. If you like to introduce sessionToken please open one PR for it.

If you want to introduce base, another PR.

Copy link
Member

Choose a reason for hiding this comment

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

(TODO for myself): this should be auto generated

@pi0 pi0 marked this pull request as draft February 3, 2025 09:40
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