Move goog.i18n deps to a module #8360
Conversation
rasom
requested review from
antdanchenko,
churik,
jakubgs and
a team
as code owners
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (64)
|
rasom
changed the title
rasom
force-pushed
the
goog.i18n-module
branch
2 times, most recently
from
1ddb1e3
to
ac10daa
Compare
rasom
changed the title
I doubt that it will have any effect in runtime. |
|
@rasom ok thanks. btw I've read that type hinting in cljs is only useful for booleans, I wonder if we are using enough of them for it to be worth doing |
100% of end-end tests have passedPassed tests (49)Click to expand |
Makefile
Outdated
| @@ -100,16 +100,22 @@ prod-build: | |||
|
|
|||
| prod-build-android: export TARGET_OS ?= android | |||
| prod-build-android: | |||
| lein prod-build-android | |||
| cp params_prod.edn params.edn && \ | |||
There was a problem hiding this comment.
Should this cp be here? Why not make it e separate target? Like:
prod-params:
cp params_prod.edn params.edn
prod-build-android: prod-params
Or something like that.
Also, can you explain what this file changes exactly? Maybe you can add that as a comment to the Makefile target?
There was a problem hiding this comment.
This file changes the parameter which makes macro behave differently in prod and dev env.
There was a problem hiding this comment.
Also, who use these files? Is there any reason why this can't be managed by env variables?
There was a problem hiding this comment.
currently defmodule macro uses this file (introduced in this PR), another macro will need platform version in that file. This is necessary to provide env vars during cljs compile time. It can be done via env vars though might be a bit implicit. I would merge it as it is and then spend time on this later, because I'm kind of have a bit different objective. It works and I'm happy.
There was a problem hiding this comment.
Sure, just don't want to multiply entities in the repo if it's not necessary. And it doesn't have to be implicit, you can clearly define the env variable above the Makefile target and make it very clear so as to what it does, like so:
prod-build-android: export TARGET_OS ?= android
prod-build-android: export BUILD_ENV ?= prod
prod-build-android:
lein prod-build-android && \
node prepare-modules.jsWe also have .env* files in the repo root which contian various settings like that, might be a good place for that too.
There was a problem hiding this comment.
for some reason BUILD_ENV is not seen from lein if defined this way, only if I make it BUILD_ENV=prod lein prod-build-android && \
There was a problem hiding this comment.
Might be because of --pure.
https://github.com/status-im/status-react/blob/cdae3423bcc986d9a390de0a5479cfa4c005c121/nix/shell.sh#L34-L38
For now the simplest thing to do would be to just add a --keep BUILD_ENV somewhere in there. What do you think @pombeirp ?
I can do a separate PR that will add an env variables to do that.
There was a problem hiding this comment.
I've merged #8398 so try rebasing and adding an export NIX_KEEP ?= 'BUILD_ENV' somewhere at the top of the Makefile, probably next to this:
https://github.com/status-im/status-react/blob/0e7f73fd11e396c5e5fa898178d3872eb676f234/Makefile#L29
And add a comment saying something like:
# Defined which variables will be kept for Nix pure shell, use semicolon as dividerAnd we should be golden.
|
This PR now has conflicts due to #8360 being merged. |
|
@jakubgs rebased |
100% of end-end tests have passedPassed tests (49)Click to expand |
100% of end-end tests have passedPassed tests (49)Click to expand |
|
No regression found.
|
This PR adds the first cljs module to the project and introduces
defmodulemacro which makes definition of module's api and loading of module a bit simpler.Moving all
goog/i18nmentions to a separate module makesindex.android.js8.91% smaller (2729KB vs 2996KB) and startup ~4% faster. The loading of the module even on a slow device is quite fast (90ms), assuming its size is ~216KB. Probably it is smaller and faster than it was before this PR because I rewrote it a bit so that compilation warning doesn't appear anymore, and thusgood/i18ncalls are now optimized better during compilation.This warning is fixed:
Loading of module on Samsung a500d:
status: ready