GH-34567: [JS] Improve build and do not generate `bin/bin` directory #36607

abetomo · 2023-07-11T03:00:12Z

Rationale for this change

bin/bin directory is unnecessary and should not be generated.

What changes are included in this PR?

Add setting to exclude in tsconfig
Correctly set up bin out directory

Are these changes tested?

The following files are not generated.

targets/apache-arrow/bin/bin/arrow2csv.js
targets/apache-arrow/bin/bin/arrow2csv.js.map
targets/apache-arrow/bin/bin/arrow2csv.mjs
targets/apache-arrow/bin/src/bin/arrow2csv.ts
targets/es2015/cjs/bin/bin/arrow2csv.js
targets/es2015/cjs/bin/bin/arrow2csv.js.map
targets/es2015/cjs/bin/src/bin/arrow2csv.ts
targets/es2015/esm/bin/bin/arrow2csv.js
targets/es2015/esm/bin/bin/arrow2csv.js.map
targets/es2015/esm/bin/src/bin/arrow2csv.ts
targets/es2015/umd/bin/bin/arrow2csv.js
targets/es2015/umd/bin/bin/arrow2csv.js.map
targets/es2015/umd/bin/src/bin/arrow2csv.ts
targets/es5/cjs/bin/bin/arrow2csv.js
targets/es5/cjs/bin/bin/arrow2csv.js.map
targets/es5/cjs/bin/src/bin/arrow2csv.ts
targets/es5/esm/bin/bin/arrow2csv.js
targets/es5/esm/bin/bin/arrow2csv.js.map
targets/es5/esm/bin/src/bin/arrow2csv.ts
targets/es5/umd/bin/bin/arrow2csv.js
targets/es5/umd/bin/bin/arrow2csv.js.map
targets/es5/umd/bin/src/bin/arrow2csv.ts
targets/esnext/cjs/bin/bin/arrow2csv.js
targets/esnext/cjs/bin/bin/arrow2csv.js.map
targets/esnext/cjs/bin/src/bin/arrow2csv.ts
targets/esnext/esm/bin/bin/arrow2csv.js
targets/esnext/esm/bin/bin/arrow2csv.js.map
targets/esnext/esm/bin/src/bin/arrow2csv.ts
targets/esnext/umd/bin/bin/arrow2csv.js
targets/esnext/umd/bin/bin/arrow2csv.js.map
targets/esnext/umd/bin/src/bin/arrow2csv.ts

Are there any user-facing changes?

Closes: [JS] Remove bin/bin directory from bundles #34567

trxcllnt

I think the reason we were compiling these separately was because they are run by npm/node directly and aren't typically run with special flags or transpiled in via a bundler. Therefore, we chose to compile the bin files down to code that runs on the lowest-common-denominator node version.

For example, someone may run their app with node --loader ts-node/esm/transpile-only, but they usually run bin scripts via npm run foo or npx arrow2csv.

Transpiling these scripts into <target>/bin/bin/ is definitely a bug/oversight. My guess is this was inadvertently introduced as part of @domoritz's sourcemap/mjs fixes a while back?

Regardless, I'd vote we keep the special casing and just fix the logic to transpile them down to ES5 + CJS. Thoughts @domoritz?

abetomo · 2023-07-13T12:33:25Z

Thank you for your comment.
I got it.

By the way, is it expected that targets/apache-arrow/bin/* has the following files?

targets/apache-arrow/bin/arrow2csv.js
targets/apache-arrow/bin/arrow2csv.js.map
targets/apache-arrow/bin/arrow2csv.mjs

Or is it this?

targets/apache-arrow/bin/arrow2csv.d.ts
targets/apache-arrow/bin/arrow2csv.d.ts.map
targets/apache-arrow/bin/arrow2csv.js
targets/apache-arrow/bin/arrow2csv.js.map
targets/apache-arrow/bin/arrow2csv.mjs
targets/apache-arrow/bin/arrow2csv.mjs.map

abetomo · 2023-07-16T01:08:55Z

I have fixed it. What do you think of this patch?
223e9d2

bin directory is as follows.

targets/apache-arrow/bin/arrow2csv.js
targets/apache-arrow/bin/arrow2csv.js.map
targets/apache-arrow/bin/arrow2csv.mjs
targets/es2015/cjs/bin/arrow2csv.js
targets/es2015/cjs/bin/arrow2csv.js.map
targets/es2015/esm/bin/arrow2csv.js
targets/es2015/esm/bin/arrow2csv.js.map
targets/es2015/umd/bin/arrow2csv.js
targets/es2015/umd/bin/arrow2csv.js.map
targets/es5/cjs/bin/arrow2csv.js
targets/es5/cjs/bin/arrow2csv.js.map
targets/es5/esm/bin/arrow2csv.js
targets/es5/esm/bin/arrow2csv.js.map
targets/es5/umd/bin/arrow2csv.js
targets/es5/umd/bin/arrow2csv.js.map
targets/esnext/cjs/bin/arrow2csv.js
targets/esnext/cjs/bin/arrow2csv.js.map
targets/esnext/esm/bin/arrow2csv.js
targets/esnext/esm/bin/arrow2csv.js.map
targets/esnext/umd/bin/arrow2csv.js
targets/esnext/umd/bin/arrow2csv.js.map
targets/ts/bin/arrow2csv.ts

abetomo · 2023-07-16T01:15:50Z

Oops.
That's an error in lint. I will check it.

Is #36607 (comment) as expected if I only look at the files in the bin directory?

trxcllnt · 2023-07-17T17:50:36Z

@abetomo the list in #36607 (comment) looks close.

I don't think we need the .d.ts typings in the package bin directory
Do we need both the .js.map and .mjs.map in the targets/apache-arrow/bin/ dir?
I think the TS source package (targets/ts/bin) should still get a transpiled arrow2csv.js

abetomo · 2023-07-20T00:20:50Z

Thanks for the review.
I have fixed it.
bin directory is as follows

targets/apache-arrow/bin/arrow2csv.js
targets/apache-arrow/bin/arrow2csv.mjs
targets/es2015/cjs/bin/arrow2csv.js
targets/es2015/esm/bin/arrow2csv.js
targets/es2015/umd/bin/arrow2csv.js
targets/es5/cjs/bin/arrow2csv.js
targets/es5/esm/bin/arrow2csv.js
targets/es5/umd/bin/arrow2csv.js
targets/esnext/cjs/bin/arrow2csv.js
targets/esnext/esm/bin/arrow2csv.js
targets/esnext/umd/bin/arrow2csv.js
targets/ts/bin/arrow2csv.ts

I ran it.

node targets/<PATH>/bin/arrow2csv.js

Help display. (Success.)

targets/apache-arrow/bin/arrow2csv.js
targets/apache-arrow/bin/arrow2csv.mjs
targets/es2015/cjs/bin/arrow2csv.js
targets/es2015/esm/bin/arrow2csv.js
targets/es5/cjs/bin/arrow2csv.js
targets/es5/esm/bin/arrow2csv.js
targets/esnext/cjs/bin/arrow2csv.js
targets/esnext/esm/bin/arrow2csv.js

Error

targets/es2015/umd/bin/arrow2csv.js
targets/es5/umd/bin/arrow2csv.js
targets/esnext/umd/bin/arrow2csv.js

Error message.

node:internal/modules/cjs/loader:1073
  throw err;
  ^

Error: Cannot find module '../util/pretty.js'
Require stack:
- /.../arrow/js/targets/es2015/umd/bin/arrow2csv.js
    at Module._resolveFilename (node:internal/modules/cjs/loader:1070:15)
    at Module._load (node:internal/modules/cjs/loader:923:27)
    at Module.require (node:internal/modules/cjs/loader:1137:19)
    at require (node:internal/modules/helpers:121:18)
    at Object.<anonymous> (/.../arrow/js/targets/es2015/umd/bin/arrow2csv.js:24:21)
    at Module._compile (node:internal/modules/cjs/loader:1255:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1309:10)
    at Module.load (node:internal/modules/cjs/loader:1113:32)
    at Module._load (node:internal/modules/cjs/loader:960:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/.../arrow/js/targets/es2015/umd/bin/arrow2csv.js'
  ]
}

Node.js v20.3.0

Currently, targets/*/umd/ only has Arrow.js, Arrow.externs.js and Arrow.es2015.min.js.map because we don't build them.
Should I also build the required libraries?

About `ts`

Currently, there are only *.ts in targets/ts/.
If we build targets/ts/bin/arrow2csv.js, we also need to build other files, what should we do?

domoritz · 2023-07-20T13:08:56Z

Should I also build the required libraries?

You mean build ../util/pretty.js? I wouldn't call those libraries but specific methods.

I would suggest that we either build the bin scripts in a way where they have all dependencies bundled or that we export the valueToString method from the arrow bundle so that we can import it in the bin script. The latter is probably a lot easier.

abetomo · 2023-07-20T22:58:45Z

Thank you for your comment.
node targets/<PATH>/umd/bin/arrow2csv.js also showed help.

…ctory Signed-off-by: abetomo <abe@enzou.tokyo>

The following error. ``` 0:0 error Parsing error: ESLint was configured to run on `<tsconfigRootDir>/src/bin/arrow2csv.ts` using `parserOptions.project`: /users/abe/work/github/arrow/js/tsconfig.json However, that TSConfig does not include this file. Either: - Change ESLint's list of included files to not include this file - Change that TSConfig to include this file - Create a new TSConfig that includes this file and include it in your parserOptions.project See the typescript-eslint docs for more info: https://typescript-eslint.io/linting/troubleshooting#i-get-errors-telling-me-eslint-was-configured-to-run--however-that-tsconfig-does-not--none-of-those-tsconfigs-include-this-file ``` Signed-off-by: abetomo <abe@enzou.tokyo>

Signed-off-by: abetomo <abe@enzou.tokyo>

trxcllnt · 2023-09-14T22:55:38Z

@abetomo thanks for the contribution. I rebased everything on top of main and added a transpiled version to the TS package. I also verified running arrow2csv in each of the targets worked:

for x in apache-arrow ts; do
  ./targets/$x/bin/arrow2csv.js -f test/data/cpp/stream/dictionary.arrow || exit 1;
done
for x in es5 es2015 esnext; do
  for y in cjs esm umd; do
    ./targets/$x/$y/bin/arrow2csv.js -f test/data/cpp/stream/dictionary.arrow || exit 1;
  done
done

kou · 2023-09-15T01:40:52Z

Can we merge this?

conbench-apache-arrow · 2023-09-15T20:08:44Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit a446ff7.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

…ctory (apache#36607) ### Rationale for this change `bin/bin` directory is unnecessary and should not be generated. ### What changes are included in this PR? * Add setting to exclude in tsconfig * Correctly set up `bin` out directory ### Are these changes tested? The following files are not generated. ``` targets/apache-arrow/bin/bin/arrow2csv.js targets/apache-arrow/bin/bin/arrow2csv.js.map targets/apache-arrow/bin/bin/arrow2csv.mjs targets/apache-arrow/bin/src/bin/arrow2csv.ts targets/es2015/cjs/bin/bin/arrow2csv.js targets/es2015/cjs/bin/bin/arrow2csv.js.map targets/es2015/cjs/bin/src/bin/arrow2csv.ts targets/es2015/esm/bin/bin/arrow2csv.js targets/es2015/esm/bin/bin/arrow2csv.js.map targets/es2015/esm/bin/src/bin/arrow2csv.ts targets/es2015/umd/bin/bin/arrow2csv.js targets/es2015/umd/bin/bin/arrow2csv.js.map targets/es2015/umd/bin/src/bin/arrow2csv.ts targets/es5/cjs/bin/bin/arrow2csv.js targets/es5/cjs/bin/bin/arrow2csv.js.map targets/es5/cjs/bin/src/bin/arrow2csv.ts targets/es5/esm/bin/bin/arrow2csv.js targets/es5/esm/bin/bin/arrow2csv.js.map targets/es5/esm/bin/src/bin/arrow2csv.ts targets/es5/umd/bin/bin/arrow2csv.js targets/es5/umd/bin/bin/arrow2csv.js.map targets/es5/umd/bin/src/bin/arrow2csv.ts targets/esnext/cjs/bin/bin/arrow2csv.js targets/esnext/cjs/bin/bin/arrow2csv.js.map targets/esnext/cjs/bin/src/bin/arrow2csv.ts targets/esnext/esm/bin/bin/arrow2csv.js targets/esnext/esm/bin/bin/arrow2csv.js.map targets/esnext/esm/bin/src/bin/arrow2csv.ts targets/esnext/umd/bin/bin/arrow2csv.js targets/esnext/umd/bin/bin/arrow2csv.js.map targets/esnext/umd/bin/src/bin/arrow2csv.ts ``` ### Are there any user-facing changes? * Closes: apache#34567 Lead-authored-by: abetomo <abe@enzou.tokyo> Co-authored-by: ptaylor <paul.e.taylor@me.com> Signed-off-by: Dominik Moritz <domoritz@gmail.com>

abetomo requested review from domoritz and trxcllnt as code owners July 11, 2023 03:00

github-actions bot added Component: JavaScript awaiting review Awaiting review labels Jul 11, 2023

trxcllnt requested changes Jul 11, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jul 11, 2023

abetomo force-pushed the improve-builds branch from 3d36e8c to 223e9d2 Compare July 16, 2023 01:06

trxcllnt requested review from assignUser, kou, raulcd, paleolimbot, thisisnic, wjones127, lidavidm, kevingurney, zeroshade, westonpace and wgtmac as code owners September 14, 2023 22:49

abetomo added 4 commits September 14, 2023 15:49

apacheGH-34567: [JS] Improve build and do not generate bin/bin dire…

22ac721

…ctory Signed-off-by: abetomo <abe@enzou.tokyo>

Do not generate map files

b80cad2

Signed-off-by: abetomo <abe@enzou.tokyo>

Export valueToString from Arrow

d1f8961

Signed-off-by: abetomo <abe@enzou.tokyo>

github-actions bot removed Component: Python Component: C# Component: Ruby Component: FlightRPC Component: C++ - Gandiva Component: GLib Component: MATLAB Component: Documentation Component: Swift labels Sep 14, 2023

trxcllnt requested review from trxcllnt and removed request for lidavidm, zeroshade, raulcd, westonpace, wgtmac, wjones127, kevingurney, paleolimbot, thisisnic, assignUser and kou September 14, 2023 22:51

trxcllnt approved these changes Sep 14, 2023

View reviewed changes

domoritz merged commit a446ff7 into apache:main Sep 15, 2023
11 checks passed

domoritz removed the awaiting committer review Awaiting committer review label Sep 15, 2023

abetomo deleted the improve-builds branch September 15, 2023 08:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-34567: [JS] Improve build and do not generate `bin/bin` directory #36607

GH-34567: [JS] Improve build and do not generate `bin/bin` directory #36607

abetomo commented Jul 11, 2023 •

edited by kou

trxcllnt left a comment

abetomo commented Jul 13, 2023

abetomo commented Jul 16, 2023

abetomo commented Jul 16, 2023

trxcllnt commented Jul 17, 2023

abetomo commented Jul 20, 2023

domoritz commented Jul 20, 2023

abetomo commented Jul 20, 2023

trxcllnt commented Sep 14, 2023 •

edited

kou commented Sep 15, 2023

conbench-apache-arrow bot commented Sep 15, 2023

GH-34567: [JS] Improve build and do not generate bin/bin directory #36607

GH-34567: [JS] Improve build and do not generate bin/bin directory #36607

Conversation

abetomo commented Jul 11, 2023 • edited by kou

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

trxcllnt left a comment

Choose a reason for hiding this comment

abetomo commented Jul 13, 2023

abetomo commented Jul 16, 2023

abetomo commented Jul 16, 2023

trxcllnt commented Jul 17, 2023

abetomo commented Jul 20, 2023

I ran it.

Help display. (Success.)

Error

About ts

domoritz commented Jul 20, 2023

abetomo commented Jul 20, 2023

trxcllnt commented Sep 14, 2023 • edited

kou commented Sep 15, 2023

conbench-apache-arrow bot commented Sep 15, 2023

GH-34567: [JS] Improve build and do not generate `bin/bin` directory #36607

GH-34567: [JS] Improve build and do not generate `bin/bin` directory #36607

abetomo commented Jul 11, 2023 •

edited by kou

About `ts`

trxcllnt commented Sep 14, 2023 •

edited