You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/04/09 04:27:29 UTC
[GitHub] [arrow] domoritz opened a new pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
domoritz opened a new pull request #9961:
URL: https://github.com/apache/arrow/pull/9961
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] domoritz commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
domoritz commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r610802832
##########
File path: js/gulp/compile-task.js
##########
@@ -25,12 +25,12 @@ const typescriptTask = require('./typescript-task');
const { arrowTask, arrowTSTask } = require('./arrow-task');
const compileTask = ((cache) => memoizeTask(cache, function compile(target, format, ...args) {
- return target === `src` ? Observable.empty()
- : target === npmPkgName ? arrowTask(target, format, ...args)()
- : target === `ts` ? arrowTSTask(target, format, ...args)()
- : format === `umd` ? target === `es5` ? closureTask(target, format, ...args)()
- : minifyTask(target, format, ...args)()
- : typescriptTask(target, format, ...args)();
+ return target === `src` ? Observable.empty()
+ : target === npmPkgName ? arrowTask(target, format, ...args)()
+ : target === `ts` ? arrowTSTask(target, format, ...args)()
+ : format === `umd` ? target === `es2015` ? closureTask(target, format, ...args)()
Review comment:
I don't know (closure compiler is a mystery to be tbh). I thought this may be needed to switch to es2015 but I'll undo this for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] trxcllnt commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
trxcllnt commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r610813521
##########
File path: js/gulp/compile-task.js
##########
@@ -25,12 +25,12 @@ const typescriptTask = require('./typescript-task');
const { arrowTask, arrowTSTask } = require('./arrow-task');
const compileTask = ((cache) => memoizeTask(cache, function compile(target, format, ...args) {
- return target === `src` ? Observable.empty()
- : target === npmPkgName ? arrowTask(target, format, ...args)()
- : target === `ts` ? arrowTSTask(target, format, ...args)()
- : format === `umd` ? target === `es5` ? closureTask(target, format, ...args)()
- : minifyTask(target, format, ...args)()
- : typescriptTask(target, format, ...args)();
+ return target === `src` ? Observable.empty()
+ : target === npmPkgName ? arrowTask(target, format, ...args)()
+ : target === `ts` ? arrowTSTask(target, format, ...args)()
+ : format === `umd` ? target === `es2015` ? closureTask(target, format, ...args)()
Review comment:
Closure compiler is a mystery to me as well :joy:. I have to doc-dive every time we update to find the new combinations of magic strings should be. lol yuck
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#issuecomment-816392424
https://issues.apache.org/jira/browse/ARROW-12309
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] trxcllnt commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
trxcllnt commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r610380913
##########
File path: js/gulp/compile-task.js
##########
@@ -25,12 +25,12 @@ const typescriptTask = require('./typescript-task');
const { arrowTask, arrowTSTask } = require('./arrow-task');
const compileTask = ((cache) => memoizeTask(cache, function compile(target, format, ...args) {
- return target === `src` ? Observable.empty()
- : target === npmPkgName ? arrowTask(target, format, ...args)()
- : target === `ts` ? arrowTSTask(target, format, ...args)()
- : format === `umd` ? target === `es5` ? closureTask(target, format, ...args)()
- : minifyTask(target, format, ...args)()
- : typescriptTask(target, format, ...args)();
+ return target === `src` ? Observable.empty()
+ : target === npmPkgName ? arrowTask(target, format, ...args)()
+ : target === `ts` ? arrowTSTask(target, format, ...args)()
+ : format === `umd` ? target === `es2015` ? closureTask(target, format, ...args)()
Review comment:
Does Closure finally produce good ES2015+? I think we've only been using it to make the ES5 UMD bundle because it either was bad, or didn't always work to compile ES2015+.
If it does, I'd be happy to rip out Webpack + Terser and use Closure for all the UMD bundles.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] trxcllnt commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
trxcllnt commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r610381787
##########
File path: js/README.md
##########
@@ -224,7 +224,7 @@ The base `apache-arrow` package includes all the compilation targets for conveni
The targets are also published under the `@apache-arrow` namespace:
```sh
-npm install apache-arrow # <-- combined es5/UMD, esnext/CommonJS/ESModules/UMD, and TypeScript package
+npm install apache-arrow # <-- combined es2015/UMD, esnext/CommonJS/ESModules/UMD, and TypeScript package
Review comment:
While we're editing this line, might as well make it reflect reality too:
```suggestion
npm install apache-arrow # <-- combined es2015/UMD + esNext/CommonJS/ESModules/UMD
```
##########
File path: js/README.md
##########
@@ -224,7 +224,7 @@ The base `apache-arrow` package includes all the compilation targets for conveni
The targets are also published under the `@apache-arrow` namespace:
```sh
-npm install apache-arrow # <-- combined es5/UMD, esnext/CommonJS/ESModules/UMD, and TypeScript package
+npm install apache-arrow # <-- combined es2015/UMD, esnext/CommonJS/ESModules/UMD, and TypeScript package
Review comment:
While we're editing this line, might as well make it reflect reality too:
```suggestion
npm install apache-arrow # <-- combined es2015/UMD + esnext/CommonJS/ESModules/UMD
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] domoritz commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
domoritz commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r612052131
##########
File path: js/bin/integration.js
##########
@@ -30,7 +30,7 @@ const {
Table,
RecordBatchReader,
util: { createElementComparator }
-} = require('../targets/apache-arrow/Arrow.es5.min');
+} = require('../targets/apache-arrow/Arrow.es2015.min');
Review comment:
It looks like the problem is actually that the es2015 bundles expect `window` to be defined. I switched to using cjs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] domoritz commented on pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
domoritz commented on pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#issuecomment-817366232
I didn't mean not having bundles. I mean that the bundle will default to es2015. The es5 bundle wouldn't be as optimal anymore but the es5 bundles don't seem to get much traffic anyway: https://www.npmjs.com/search?q=apache-arrow%2Fes5.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] domoritz commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
domoritz commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r610347626
##########
File path: js/gulp/compile-task.js
##########
@@ -25,12 +25,12 @@ const typescriptTask = require('./typescript-task');
const { arrowTask, arrowTSTask } = require('./arrow-task');
const compileTask = ((cache) => memoizeTask(cache, function compile(target, format, ...args) {
- return target === `src` ? Observable.empty()
- : target === npmPkgName ? arrowTask(target, format, ...args)()
- : target === `ts` ? arrowTSTask(target, format, ...args)()
- : format === `umd` ? target === `es5` ? closureTask(target, format, ...args)()
- : minifyTask(target, format, ...args)()
- : typescriptTask(target, format, ...args)();
+ return target === `src` ? Observable.empty()
+ : target === npmPkgName ? arrowTask(target, format, ...args)()
+ : target === `ts` ? arrowTSTask(target, format, ...args)()
+ : format === `umd` ? target === `es2015` ? closureTask(target, format, ...args)()
Review comment:
@trxcllnt Does this look right to you?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] domoritz commented on pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
domoritz commented on pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#issuecomment-817342595
I wonder, would it make sense to remove closure compiler if we only use it for es5 and es5 isn't the default bundle anymore? Is the benefit for users big enough to warrant the overhead for devs?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] domoritz commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
domoritz commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r610908549
##########
File path: js/bin/integration.js
##########
@@ -30,7 +30,7 @@ const {
Table,
RecordBatchReader,
util: { createElementComparator }
-} = require('../targets/apache-arrow/Arrow.es5.min');
+} = require('../targets/apache-arrow/Arrow.es2015.min');
Review comment:
It's supposed to use node 14: https://github.com/apache/arrow/blob/113a515e2b84547e5b198fa5d0c489bf363ea46b/ci/docker/conda-integration.dockerfile#L24
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] trxcllnt commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
trxcllnt commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r610381787
##########
File path: js/README.md
##########
@@ -224,7 +224,7 @@ The base `apache-arrow` package includes all the compilation targets for conveni
The targets are also published under the `@apache-arrow` namespace:
```sh
-npm install apache-arrow # <-- combined es5/UMD, esnext/CommonJS/ESModules/UMD, and TypeScript package
+npm install apache-arrow # <-- combined es2015/UMD, esnext/CommonJS/ESModules/UMD, and TypeScript package
Review comment:
```suggestion
npm install apache-arrow # <-- combined ES2015/UMD + ESNext/CommonJS/ESModules/UMD
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] domoritz commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
domoritz commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r610803828
##########
File path: js/bin/integration.js
##########
@@ -30,7 +30,7 @@ const {
Table,
RecordBatchReader,
util: { createElementComparator }
-} = require('../targets/apache-arrow/Arrow.es5.min');
+} = require('../targets/apache-arrow/Arrow.es2015.min');
Review comment:
Would that mean we need to build es5 and es2015 bundles in the arrow package?
I would hope that our CI doesn't use a version that only supports es5 anymore. I'd say we should update the CI if it does.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] domoritz commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
domoritz commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r610806291
##########
File path: js/gulp/compile-task.js
##########
@@ -25,12 +25,12 @@ const typescriptTask = require('./typescript-task');
const { arrowTask, arrowTSTask } = require('./arrow-task');
const compileTask = ((cache) => memoizeTask(cache, function compile(target, format, ...args) {
- return target === `src` ? Observable.empty()
- : target === npmPkgName ? arrowTask(target, format, ...args)()
- : target === `ts` ? arrowTSTask(target, format, ...args)()
- : format === `umd` ? target === `es5` ? closureTask(target, format, ...args)()
- : minifyTask(target, format, ...args)()
- : typescriptTask(target, format, ...args)();
+ return target === `src` ? Observable.empty()
+ : target === npmPkgName ? arrowTask(target, format, ...args)()
+ : target === `ts` ? arrowTSTask(target, format, ...args)()
+ : format === `umd` ? target === `es2015` ? closureTask(target, format, ...args)()
Review comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] trxcllnt commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
trxcllnt commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r610813521
##########
File path: js/gulp/compile-task.js
##########
@@ -25,12 +25,12 @@ const typescriptTask = require('./typescript-task');
const { arrowTask, arrowTSTask } = require('./arrow-task');
const compileTask = ((cache) => memoizeTask(cache, function compile(target, format, ...args) {
- return target === `src` ? Observable.empty()
- : target === npmPkgName ? arrowTask(target, format, ...args)()
- : target === `ts` ? arrowTSTask(target, format, ...args)()
- : format === `umd` ? target === `es5` ? closureTask(target, format, ...args)()
- : minifyTask(target, format, ...args)()
- : typescriptTask(target, format, ...args)();
+ return target === `src` ? Observable.empty()
+ : target === npmPkgName ? arrowTask(target, format, ...args)()
+ : target === `ts` ? arrowTSTask(target, format, ...args)()
+ : format === `umd` ? target === `es2015` ? closureTask(target, format, ...args)()
Review comment:
Closure compiler is a mystery to me as well :joy:. I have to doc-dive every time we update to find what the new magic string combinations are. lol yuck
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] domoritz commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
domoritz commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r611794871
##########
File path: js/gulpfile.js
##########
@@ -56,13 +56,13 @@ knownTargets.forEach((target) => {
));
});
-// The main "apache-arrow" module builds the es5/umd, esnext/cjs,
+// The main "apache-arrow" module builds the es2015/umd, esnext/cjs,
// esnext/esm, and esnext/umd targets, then copies and renames the
// compiled output into the apache-arrow folder
gulp.task(`build:${npmPkgName}`,
gulp.series(
gulp.parallel(
- `build:${taskName(`es5`, `umd`)}`,
+ `build:${taskName(`es2015`, `umd`)}`,
Review comment:
@trxcllnt I was hoping this would be the main thing to do but the bundle isn't right.
Why is there no es2015 bundle? Why is there an externs file for es5?
<img width="368" alt="Screen Shot 2021-04-12 at 09 48 01" src="https://user-images.githubusercontent.com/589034/114431287-2d182200-9b74-11eb-8b8c-341e7cff8d94.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] domoritz commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
domoritz commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r610908549
##########
File path: js/bin/integration.js
##########
@@ -30,7 +30,7 @@ const {
Table,
RecordBatchReader,
util: { createElementComparator }
-} = require('../targets/apache-arrow/Arrow.es5.min');
+} = require('../targets/apache-arrow/Arrow.es2015.min');
Review comment:
It's supposed to use node 14: https://github.com/apache/arrow/blob/113a515e2b84547e5b198fa5d0c489bf363ea46b/ci/docker/conda-integration.dockerfile#L24
The test failure looks unrelated again.
<img width="1299" alt="Screen Shot 2021-04-09 at 14 13 52" src="https://user-images.githubusercontent.com/589034/114241444-fbb61100-993d-11eb-9cf5-2b2d9298793a.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] domoritz commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
domoritz commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r612519919
##########
File path: js/bin/integration.js
##########
@@ -30,7 +30,7 @@ const {
Table,
RecordBatchReader,
util: { createElementComparator }
-} = require('../targets/apache-arrow/Arrow.es5.min');
+} = require('../targets/apache-arrow/Arrow.es2015.min');
Review comment:
Works now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] domoritz commented on pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
domoritz commented on pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#issuecomment-816415923
I think I need your help, @trxcllnt, with the closure compiler here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] trxcllnt commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
trxcllnt commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r610810905
##########
File path: js/bin/integration.js
##########
@@ -30,7 +30,7 @@ const {
Table,
RecordBatchReader,
util: { createElementComparator }
-} = require('../targets/apache-arrow/Arrow.es5.min');
+} = require('../targets/apache-arrow/Arrow.es2015.min');
Review comment:
Not sure if it still does. I think the integration tests install node via conda? If this passes CI, then great. If not, we can change this to `../targets/es5/umd/Arrow` instead.
##########
File path: js/bin/integration.js
##########
@@ -30,7 +30,7 @@ const {
Table,
RecordBatchReader,
util: { createElementComparator }
-} = require('../targets/apache-arrow/Arrow.es5.min');
+} = require('../targets/apache-arrow/Arrow.es2015.min');
Review comment:
Not sure if it still does. I think the integration container installs node via conda? If this passes CI, then great. If not, we can change this to `../targets/es5/umd/Arrow` instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] trxcllnt commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
trxcllnt commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r610382954
##########
File path: js/bin/integration.js
##########
@@ -30,7 +30,7 @@ const {
Table,
RecordBatchReader,
util: { createElementComparator }
-} = require('../targets/apache-arrow/Arrow.es5.min');
+} = require('../targets/apache-arrow/Arrow.es2015.min');
Review comment:
I think we're using the ES5 bundle here because CI can potentially have (or used to have?) an old version of node.
##########
File path: js/bin/integration.js
##########
@@ -30,7 +30,7 @@ const {
Table,
RecordBatchReader,
util: { createElementComparator }
-} = require('../targets/apache-arrow/Arrow.es5.min');
+} = require('../targets/apache-arrow/Arrow.es2015.min');
Review comment:
I think we used the ES5 bundle here because CI can potentially have (or used to have?) an old version of node.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] trxcllnt commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
trxcllnt commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r610380913
##########
File path: js/gulp/compile-task.js
##########
@@ -25,12 +25,12 @@ const typescriptTask = require('./typescript-task');
const { arrowTask, arrowTSTask } = require('./arrow-task');
const compileTask = ((cache) => memoizeTask(cache, function compile(target, format, ...args) {
- return target === `src` ? Observable.empty()
- : target === npmPkgName ? arrowTask(target, format, ...args)()
- : target === `ts` ? arrowTSTask(target, format, ...args)()
- : format === `umd` ? target === `es5` ? closureTask(target, format, ...args)()
- : minifyTask(target, format, ...args)()
- : typescriptTask(target, format, ...args)();
+ return target === `src` ? Observable.empty()
+ : target === npmPkgName ? arrowTask(target, format, ...args)()
+ : target === `ts` ? arrowTSTask(target, format, ...args)()
+ : format === `umd` ? target === `es2015` ? closureTask(target, format, ...args)()
Review comment:
Does Closure finally produce good ES2015+? I think we've only been using it to make the ES5 UMD bundle because it was either bad, or didn't always work when `language_out` was ES2015+.
If it does, I'd be happy to rip out Webpack + Terser and use Closure for all the UMD bundles.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou closed pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
kou closed pull request #9961:
URL: https://github.com/apache/arrow/pull/9961
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] trxcllnt commented on a change in pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
trxcllnt commented on a change in pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#discussion_r610380913
##########
File path: js/gulp/compile-task.js
##########
@@ -25,12 +25,12 @@ const typescriptTask = require('./typescript-task');
const { arrowTask, arrowTSTask } = require('./arrow-task');
const compileTask = ((cache) => memoizeTask(cache, function compile(target, format, ...args) {
- return target === `src` ? Observable.empty()
- : target === npmPkgName ? arrowTask(target, format, ...args)()
- : target === `ts` ? arrowTSTask(target, format, ...args)()
- : format === `umd` ? target === `es5` ? closureTask(target, format, ...args)()
- : minifyTask(target, format, ...args)()
- : typescriptTask(target, format, ...args)();
+ return target === `src` ? Observable.empty()
+ : target === npmPkgName ? arrowTask(target, format, ...args)()
+ : target === `ts` ? arrowTSTask(target, format, ...args)()
+ : format === `umd` ? target === `es2015` ? closureTask(target, format, ...args)()
Review comment:
Does Closure finally produce good ES2015+? I think we've only been using it to make the ES5 UMD bundle because it was either bad, or didn't always work to compile ES2015+.
If it does, I'd be happy to rip out Webpack + Terser and use Closure for all the UMD bundles.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] trxcllnt commented on pull request #9961: ARROW-12309: [JS] Make es2015 bundles the default
Posted by GitBox <gi...@apache.org>.
trxcllnt commented on pull request #9961:
URL: https://github.com/apache/arrow/pull/9961#issuecomment-817365840
@domoritz people definitely use it in jsfiddles/notebooks etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org