livekit / agents-js

Build realtime multimodal AI agents with Node.js
https://docs.livekit.io/agents
Apache License 2.0
113 stars 12 forks source link

Maximize import compatibility in example #69

Closed bcherry closed 3 weeks ago

bcherry commented 4 weeks ago

It turns out that some versions of node don't have import.meta.filename but do have import.meta.url, which can be parsed to get the filename.

This change uses the wordier-but-more-compatible version in the example.

I tested it on Node 20.11.0 (which also worked before), and Node 21.1.0 (which did not). It works on both.

changeset-bot[bot] commented 4 weeks ago

🦋 Changeset detected

Latest commit: 8c16f98c1ed5a27c3afb924f89abe741431ae1a5

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

bcherry commented 4 weeks ago

i don't know if i like this solution, it abstracts away the issue in a weird way and only for the current file scenario, while making it more complicated for other usecases.

maybe instead we could have agentFile accept either a URL or a path, and figure out on the fly which it is, translating it if necessary? passing import.meta is odd.

The issue with that is it requires the user to know whether they should use import.meta.filename or import.meta.url. I'd rather they not have to know. Another option is to leave the interface unchanged but make the examples just use agent: import.meta.filename ?? fileURLToPath(import.meta.url)

nbsp commented 4 weeks ago

i don't know if i like this solution, it abstracts away the issue in a weird way and only for the current file scenario, while making it more complicated for other usecases. maybe instead we could have agentFile accept either a URL or a path, and figure out on the fly which it is, translating it if necessary? passing import.meta is odd.

The issue with that is it requires the user to know whether they should use import.meta.filename or import.meta.url. I'd rather they not have to know. Another option is to leave the interface unchanged but make the examples just use agent: import.meta.filename ?? fileURLToPath(import.meta.url)

i think the obvious solution would be to tell users to do fileURLToPath(import.meta.url), not mentioning the other one. this works across versions, the other one doesn't, if they know what it is they'll know where to use it.

bcherry commented 4 weeks ago

i don't know if i like this solution, it abstracts away the issue in a weird way and only for the current file scenario, while making it more complicated for other usecases. maybe instead we could have agentFile accept either a URL or a path, and figure out on the fly which it is, translating it if necessary? passing import.meta is odd.

The issue with that is it requires the user to know whether they should use import.meta.filename or import.meta.url. I'd rather they not have to know. Another option is to leave the interface unchanged but make the examples just use agent: import.meta.filename ?? fileURLToPath(import.meta.url)

i think the obvious solution would be to tell users to do fileURLToPath(import.meta.url), not mentioning the other one. this works across versions, the other one doesn't, if they know what it is they'll know where to use it.

true that makes sense too

bcherry commented 3 weeks ago

potentially also worth adding a check on the job side, that errors if import(file).then((mod) => mod.default) is undefined? other than that lg

yep good idea - done