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

[JS] Fix instanceof or move away from instanceof within arrow-js #40748

Open
westonpace opened this issue Mar 22, 2024 · 0 comments
Open

[JS] Fix instanceof or move away from instanceof within arrow-js #40748

westonpace opened this issue Mar 22, 2024 · 0 comments

Comments

@westonpace
Copy link
Member

westonpace commented Mar 22, 2024

Describe the enhancement requested

There are a number of places that arrow's js package uses instanceof. For example:

// from table.ts in the Table constructor
if (args[0] instanceof Schema) {
    schema = args.shift() as Schema<T>;
}

Unfortunately, this is quite brittle. This will fail whenever a library ends up with multiple instances of the arrow library. For example:

  • LanceDb currently uses arrow version 14. If the calling user is using any other version of arrow then node will happily install two versions of arrow and the checks will fail. (also, these failures tend to be very opaque)
  • Even if the user is using the exact same version of arrow there are cases where two instances of arrow are created. For example, our users using vite have run into Pre-bundled dependencies doesn't dedupe imports in external files vitejs/vite#3910

I would argue that some of the core types (at least Schema but maybe also Table / Vector / RecordBatch too) should be considered stable and users should be allowed to have different versions / instances. We tried working around this in lancedb by declaring arrow a peer dependency and that solves the first bullet above but it's also somewhat unconventional (for javascript) and inconvenient for our users (they are forced to use whatever version of arrow we are using and when we change our arrow version it is a breaking change).

This is not critical, we've worked around this by re-building the schema if the input from our user looks like a schema, but I wanted to see if other JS users/maintainers felt similarly and, if so, I might try and find some time to explore a fix.

Some potential fixes:

Component(s)

JavaScript

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant