apache / arrow

Apache Arrow is a multi-language toolbox for accelerated data interchange and in-memory processing
https://arrow.apache.org/
Apache License 2.0
13.87k stars 3.38k forks source link

[JS] Avoid circular dependencies in apache-arrow #40516

Open rajsite opened 3 months ago

rajsite commented 3 months ago

Describe the enhancement requested

Currently the apache-arrow library has circular dependencies which cause warnings in tools like rollup:

src/example.js → dist/example.js...
(!) Circular dependencies
node_modules/apache-arrow/vector.mjs -> node_modules/apache-arrow/util/vector.mjs -> node_modules/apache-arrow/vector.mjs
node_modules/apache-arrow/vector.mjs -> node_modules/apache-arrow/util/vector.mjs -> node_modules/apache-arrow/row/map.mjs -> node_modules/apache-arrow/vector.mjs
node_modules/apache-arrow/vector.mjs -> node_modules/apache-arrow/util/vector.mjs -> node_modules/apache-arrow/row/map.mjs -> node_modules/apache-arrow/visitor/get.mjs -> node_modules/apache-arrow/vector.mjs
...and 10 more
created dist/example.js in 890ms

See the following stackblitz which reproduces that build warning: https://stackblitz.com/edit/apache-arrow-circular-dependencies?file=src%2Fexample.js

It would be nice if the library was organized such that circular dependencies could be avoided.

To workaround the warnings in rollup one can use an onwarn handler in the rollup configuration with an implementation like:

const onwarn = (warning, defaultHandler) => {
    const ignoredWarnings = [
        {
            code: 'CIRCULAR_DEPENDENCY',
            file: 'node_modules/apache-arrow'
        }
    ];

    if (
        !ignoredWarnings.some(
            ({ code, file }) => warning.code === code && warning.message.includes(file)
        )
    ) {
        defaultHandler(warning);
    }
};

Component(s)

JavaScript

kou commented 3 months ago

@domoritz Could you take a look at this?

domoritz commented 3 months ago

Thanks for the issue report. I've looked at circular deps before and luckily I think all of these are just warning and the bundler can easily serialize the dependencies. However, it would be nice to clean this up. We need to be careful not to make the library less readable by breaking up code that makes sense together into separate files.

And easy reproduction in the repo itself is to remove https://github.com/apache/arrow/blob/6b1e254f3b62924f216e06e9e563e92c69f9efd3/js/gulp/bundle-task.js#L86 and then running yarn gulp bundle:rollup. Then you get warnings for all the sample bundles we have.

I don't know how much I can make it a priority compared to other things I have on my plate. This seems like a good issue for someone else to contribute. @rajsite would you be open to making a pull request?