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 2020/10/09 22:10:14 UTC
[GitHub] [arrow] trxcllnt opened a new pull request #8418: ARROW-10255: [JS] [WIP] Reorganize exports for ESM tree-shaking
trxcllnt opened a new pull request #8418:
URL: https://github.com/apache/arrow/pull/8418
Related JIRA: https://issues.apache.org/jira/browse/ARROW-10255
Opening this as a draft PR for ongoing work to make our ESM exports more friendly to [tree-shaking](https://webpack.js.org/guides/tree-shaking/).
----------------------------------------------------------------
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 #8418: ARROW-10255: [JS] Reorganize exports for ESM tree-shaking
Posted by GitBox <gi...@apache.org>.
trxcllnt commented on pull request #8418:
URL: https://github.com/apache/arrow/pull/8418#issuecomment-814266004
cc @domoritz if you want to add a review
--
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 #8418: ARROW-10255: [JS] Reorganize exports for ESM tree-shaking
Posted by GitBox <gi...@apache.org>.
domoritz commented on a change in pull request #8418:
URL: https://github.com/apache/arrow/pull/8418#discussion_r608770963
##########
File path: js/src/util/utf8.ts
##########
@@ -22,7 +22,7 @@ import {
} from 'text-encoding-utf-8';
/** @ignore @suppress {missingRequire} */
-const _Buffer = typeof Buffer === 'function' ? Buffer : null;
+const _Buffer = eval("typeof Buffer === 'function' ? Buffer : null");
Review comment:
It would be good to try. Maybe it's okay if the function constructor never gets called.
I'm Vega, we added an ast evaluator in addition to a function constructor to support expressions in environments with CSP.
--
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 edited a comment on pull request #8418: ARROW-10255: [JS] Reorganize exports for ESM tree-shaking
Posted by GitBox <gi...@apache.org>.
trxcllnt edited a comment on pull request #8418:
URL: https://github.com/apache/arrow/pull/8418#issuecomment-815140029
@kszucs what's the current preferred way to merge PRs? Is it still the Python script that closes the JIRAs, or can we use the GitHub UI 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] alippai commented on pull request #8418: ARROW-10255: [JS] Reorganize exports for ESM tree-shaking
Posted by GitBox <gi...@apache.org>.
alippai commented on pull request #8418:
URL: https://github.com/apache/arrow/pull/8418#issuecomment-828054258
We might not need `Buffer` at all. Filed https://github.com/apache/arrow/pull/10181
--
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 #8418: ARROW-10255: [JS] Reorganize exports for ESM tree-shaking
Posted by GitBox <gi...@apache.org>.
trxcllnt commented on pull request #8418:
URL: https://github.com/apache/arrow/pull/8418#issuecomment-815140029
@kszucs what's the current preferred way to squash merge PRs now? Is it still the Python script that closes the JIRAs, or can we use the GitHub UI 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] alippai commented on pull request #8418: ARROW-10255: [JS] Reorganize exports for ESM tree-shaking
Posted by GitBox <gi...@apache.org>.
alippai commented on pull request #8418:
URL: https://github.com/apache/arrow/pull/8418#issuecomment-827972276
--
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 #8418: ARROW-10255: [JS] Reorganize exports for ESM tree-shaking
Posted by GitBox <gi...@apache.org>.
trxcllnt commented on pull request #8418:
URL: https://github.com/apache/arrow/pull/8418#issuecomment-828005715
@alippai wrapping this check in `eval` doesn't change the semantics, so that sounds like a bug in Rollup.
--
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 #8418: ARROW-10255: [JS] Reorganize exports for ESM tree-shaking
Posted by GitBox <gi...@apache.org>.
domoritz commented on a change in pull request #8418:
URL: https://github.com/apache/arrow/pull/8418#discussion_r608770963
##########
File path: js/src/util/utf8.ts
##########
@@ -22,7 +22,7 @@ import {
} from 'text-encoding-utf-8';
/** @ignore @suppress {missingRequire} */
-const _Buffer = typeof Buffer === 'function' ? Buffer : null;
+const _Buffer = eval("typeof Buffer === 'function' ? Buffer : null");
Review comment:
It would be good to try. Maybe it's okay if the function constructor never gets called.
I'm Vega, we added an ast evaluator in addition to a function constructor to support expressions in environments with CSP.
See https://vega.github.io/vega/usage/interpreter/
--
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 #8418: ARROW-10255: [JS] Reorganize exports for ESM tree-shaking
Posted by GitBox <gi...@apache.org>.
trxcllnt commented on a change in pull request #8418:
URL: https://github.com/apache/arrow/pull/8418#discussion_r608688491
##########
File path: js/src/util/utf8.ts
##########
@@ -22,7 +22,7 @@ import {
} from 'text-encoding-utf-8';
/** @ignore @suppress {missingRequire} */
-const _Buffer = typeof Buffer === 'function' ? Buffer : null;
+const _Buffer = eval("typeof Buffer === 'function' ? Buffer : null");
Review comment:
Oh I forgot about CSPs. I'll revert this for now, and change the webpack configs to specify `{ node: { buffer: false } }`.
Haven't gotten any complaints about using the Function constructor, but https://github.com/webpack/webpack/issues/5627 seems to have a few workarounds.
--
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] kszucs closed pull request #8418: ARROW-10255: [JS] Reorganize exports for ESM tree-shaking
Posted by GitBox <gi...@apache.org>.
kszucs closed pull request #8418:
URL: https://github.com/apache/arrow/pull/8418
--
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] kszucs commented on pull request #8418: ARROW-10255: [JS] Reorganize exports for ESM tree-shaking
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #8418:
URL: https://github.com/apache/arrow/pull/8418#issuecomment-815893566
@trxcllnt the python script is still the preferred one. Do you want me to merge it?
--
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 #8418: ARROW-10255: [JS] Reorganize exports for ESM tree-shaking
Posted by GitBox <gi...@apache.org>.
trxcllnt commented on pull request #8418:
URL: https://github.com/apache/arrow/pull/8418#issuecomment-815941084
@kszucs yes if you would be so kind :pray: :slightly_smiling_face:
--
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 #8418: ARROW-10255: [JS] [WIP] Reorganize exports for ESM tree-shaking
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8418:
URL: https://github.com/apache/arrow/pull/8418#issuecomment-706415050
https://issues.apache.org/jira/browse/ARROW-10255
----------------------------------------------------------------
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 #8418: ARROW-10255: [JS] Reorganize exports for ESM tree-shaking
Posted by GitBox <gi...@apache.org>.
domoritz commented on a change in pull request #8418:
URL: https://github.com/apache/arrow/pull/8418#discussion_r608352444
##########
File path: js/src/util/utf8.ts
##########
@@ -22,7 +22,7 @@ import {
} from 'text-encoding-utf-8';
/** @ignore @suppress {missingRequire} */
-const _Buffer = typeof Buffer === 'function' ? Buffer : null;
+const _Buffer = eval("typeof Buffer === 'function' ? Buffer : null");
Review comment:
I guess we are already using `new Function` so that ship has sailed?
--
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 #8418: ARROW-10255: [JS] Reorganize exports for ESM tree-shaking
Posted by GitBox <gi...@apache.org>.
domoritz commented on a change in pull request #8418:
URL: https://github.com/apache/arrow/pull/8418#discussion_r608352444
##########
File path: js/src/util/utf8.ts
##########
@@ -22,7 +22,7 @@ import {
} from 'text-encoding-utf-8';
/** @ignore @suppress {missingRequire} */
-const _Buffer = typeof Buffer === 'function' ? Buffer : null;
+const _Buffer = eval("typeof Buffer === 'function' ? Buffer : null");
Review comment:
I guess we are already using `new Function` in `createIsValidFunction` so that ship has sailed? Isn't that a problem?
--
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 #8418: ARROW-10255: [JS] Reorganize exports for ESM tree-shaking
Posted by GitBox <gi...@apache.org>.
domoritz commented on a change in pull request #8418:
URL: https://github.com/apache/arrow/pull/8418#discussion_r608350657
##########
File path: js/src/util/utf8.ts
##########
@@ -22,7 +22,7 @@ import {
} from 'text-encoding-utf-8';
/** @ignore @suppress {missingRequire} */
-const _Buffer = typeof Buffer === 'function' ? Buffer : null;
+const _Buffer = eval("typeof Buffer === 'function' ? Buffer : null");
Review comment:
This will cause problems in environments with a CSP that does not allow eval, no?
--
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