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] The package.json file incorrectly specifies sideEffects: false #38936

Open
nrabinowitz opened this issue Nov 28, 2023 · 7 comments · May be fixed by #39472, #39714 or #39953
Open

[JS] The package.json file incorrectly specifies sideEffects: false #38936

nrabinowitz opened this issue Nov 28, 2023 · 7 comments · May be fixed by #39472, #39714 or #39953

Comments

@nrabinowitz
Copy link

Describe the bug, including details regarding any error messages, version, and platform.

Problem

When building a Node application with Webpack, calls to RecordBatchStreamWriter.toNodeStream fail with the error:

Error: "toNodeStream" not available in this environment

Cause

The apache-arrow package.json file specifies "sideEffects": false, but the Node overrides required for streaming Arrow files are applied as a side effect. I believe this is true for the DOM overrides as well.

Suggested Fix

I haven't tested this, but I think that wrapping the overrides in these two files in an IIFE as is done here will fix this issue in the Webpack build.

Component(s)

JavaScript

@nrabinowitz nrabinowitz changed the title The package.json file incorrectly specifies sideEffects: false [JS] The package.json file incorrectly specifies sideEffects: false Nov 28, 2023
@domoritz
Copy link
Member

domoritz commented Jan 5, 2024

Good catch. Do you have an idea how we can test this to make sure we are doing the right fix and don't break it again?

@nrabinowitz
Copy link
Author

Good catch. Do you have an idea how we can test this to make sure we are doing the right fix and don't break it again?

I am not sure what a good test would be here. I think a minimally-reproducible case might be to build a simple script like

import {RecordBatchStreamWriter} from 'apache-arrow';
new RecordBatchStreamWriter().toNodeStream();

using WebPack, and then run the result and assert that exit code is 0. But this is admittedly painful to put into a CI test suite. If you're interested in a test like this, I could put together the example for you.

@domoritz
Copy link
Member

domoritz commented Jan 9, 2024

We already have bundler tests in https://github.com/apache/arrow/blob/b522f8c5cc152ea97d33340e3bef852e063dad46/js/test/bundle, actually. They are run in https://github.com/apache/arrow/blob/b522f8c5cc152ea97d33340e3bef852e063dad46/js/gulp/bundle-task.js and we already run the bundle in

export const execBundleTask = () => () => observableFromStreams(
. It would be great if you could put together a sample script that we ca add to the bundle folder.

@github-actions github-actions bot linked a pull request Jan 20, 2024 that will close this issue
@nrabinowitz
Copy link
Author

new RecordBatchStreamWriter().toNodeStream();

Apologies for the delay here. I've added a failing test in this PR: #39953

Let me know if there's anything else I can do to support this fix.

@domoritz
Copy link
Member

domoritz commented Feb 6, 2024

I'm pretty swamped with things right now and we won't ship a fix until the next release (arrow 16) anyway so I'll put it in my queue.

I think the approach of setting side effects in package.json is not good since it blows up bundle sizes so we should look into initializing in a class.

@ilijapuaca
Copy link
Contributor

Hey @domoritz, is there an ETA on when a patch for this may be available? It sounds like it's not considered worthy a new release, is there any timeline on v16 being published? Just wondering as this is kind of a blocker for some of our work due to no workaround being available when using esbuild.

Appreciate all the help!

@domoritz
Copy link
Member

All arrow packages I'm this repo are released together every three months.

This is an important issue but I haven't found a solution yet.

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