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

ARROW-12393: [JS] Use closure compiler for all UMD targets #10281

Closed
wants to merge 17 commits into from

Conversation

domoritz
Copy link
Member

@domoritz domoritz commented May 9, 2021

Closure compiler creates smaller bundles.

This branch

targets/es5/umd/:
695k -I Arrow.es5.min.js.map
 55k -I Arrow.externs.js
254k -I Arrow.js

targets/es2015/umd/:
534k -I Arrow.es2015.min.js.map
 55k -I Arrow.externs.js
180k -I Arrow.js

targets/esnext/umd/:
512k -I Arrow.esnext.min.js.map
 55k -I Arrow.externs.js
172k -I Arrow.js

Master (which uses closure only for es5 targets and terser otherwise)

targets/es5/umd/:
694k -I Arrow.es5.min.js.map
 55k -I Arrow.externs.js
253k -I Arrow.js

targets/es2015/umd/:
1.2M -I Arrow.es2015.min.js.map
233k -I Arrow.js

targets/esnext/umd/:
1.1M -I Arrow.esnext.min.js.map
225k -I Arrow.js

@github-actions
Copy link

github-actions bot commented May 9, 2021

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

@domoritz domoritz changed the title XXXX: [JS] Use closure for every target but es5 ARROW-12393: [JS] Use closure for every target but es5 May 9, 2021
@github-actions
Copy link

github-actions bot commented May 9, 2021

@@ -2,10 +2,10 @@
{
"extends": "./tsconfig.base.json",
"compilerOptions": {
"target": "ES2015",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trxcllnt why was this es2015? The output was always only used for closure so all .cls.json files should be identical, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently the documentation for this file was incorrect.

@domoritz
Copy link
Member Author

domoritz commented May 15, 2021

Ahh, looks like the build system tricked me.

targets/es5/umd/:
   - -I bin
1.2M -I Arrow.es5.min.js.map
302k -I Arrow.js

We should use closure for everything.

@domoritz domoritz changed the title ARROW-12393: [JS] Use closure for every target but es5 ARROW-12393: [JS] Use closure for every UMD target May 15, 2021
@domoritz
Copy link
Member Author

// Compiler configuration to build the ES2015 Closure Compiler target
is wrong then. We use the .cls config also for terser and not just closure.

domoritz and others added 3 commits May 15, 2021 09:56
Co-authored-by: P42 <72252241+p42-ai[bot]@users.noreply.github.com>
Co-authored-by: Dominik Moritz <domoritz@gmail.com>
@domoritz domoritz changed the title ARROW-12393: [JS] Use closure for every UMD target ARROW-12393: [JS] Use closure for all UMD targets May 15, 2021
@domoritz
Copy link
Member Author

I updated the pull request to use closure for every target.

@domoritz domoritz changed the title ARROW-12393: [JS] Use closure for all UMD targets ARROW-12393: [JS] Use closure compilerr for all UMD targets May 15, 2021
@domoritz domoritz changed the title ARROW-12393: [JS] Use closure compilerr for all UMD targets ARROW-12393: [JS] Use closure compiler for all UMD targets May 15, 2021
@domoritz domoritz closed this in c420ce2 Jun 3, 2021
michalursa pushed a commit to michalursa/arrow that referenced this pull request Jun 13, 2021
Closure compiler creates smaller bundles.

This branch

```
targets/es5/umd/:
695k -I Arrow.es5.min.js.map
 55k -I Arrow.externs.js
254k -I Arrow.js

targets/es2015/umd/:
534k -I Arrow.es2015.min.js.map
 55k -I Arrow.externs.js
180k -I Arrow.js

targets/esnext/umd/:
512k -I Arrow.esnext.min.js.map
 55k -I Arrow.externs.js
172k -I Arrow.js
```

Master (which uses closure only for es5 targets and terser otherwise)

```
targets/es5/umd/:
694k -I Arrow.es5.min.js.map
 55k -I Arrow.externs.js
253k -I Arrow.js

targets/es2015/umd/:
1.2M -I Arrow.es2015.min.js.map
233k -I Arrow.js

targets/esnext/umd/:
1.1M -I Arrow.esnext.min.js.map
225k -I Arrow.js
```

Closes apache#10281 from domoritz/dom/closure-everything-but-es5

Lead-authored-by: Dominik Moritz <domoritz@gmail.com>
Co-authored-by: p42-ai[bot] <72252241+p42-ai[bot]@users.noreply.github.com>
Signed-off-by: Dominik Moritz <domoritz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants