You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Possible to use Map instead of Object. Pretty much all non-IE major browsers support this, and there seem to be some performance benefits besides the obvious extra clarity of seeing that something is explicitly a mapping
Be consistent with var, let, and const. I just did some quick estimates by running grep -ri "let " empress/support_files/js/* | wc -l, etc. and found that there are 412 vars, 9 lets, and 4 consts in the Empress JS codebase. We should prooobably just make everything var.
Use foreach instead of for loops? Where possible, and where it wouldn't be an efficiency decrease. (Maps can apparently be directly iterated over, so maybe those would pair well with this.)
We may want to go about this by figuring out what browser / JS version we want to target, and then... adjusting our build setup accordingly? Maybe by adjusting jshint, or adding Babel somewhere to verify that we can use these things and still target all the environments we want to support. I'm not really sure what the best practice would be (I suspect it'd involve a lot of messing around with adding webpack or rollup to the project).
Thanks @dhakim87, @wasade, and @qiyunzhu for bringing up these points respectively during code review today!
Edit: we should also update the documentation to mention things like browser requirements, etc.
The text was updated successfully, but these errors were encountered:
From discussion with @ElDeveloper and @kwcantrell, it sounds like we're mostly moving towards just supporting Firefox / Chrome for now. So using JS features that e.g. break IE is ok.
fedarko
changed the title
Browser / ES version considerations
Document and address browser / ES version considerations
Jul 23, 2020
From code review:
Possible to use
Map
instead ofObject
. Pretty much all non-IE major browsers support this, and there seem to be some performance benefits besides the obvious extra clarity of seeing that something is explicitly a mappingBe consistent with
var
,let
, andconst
. I just did some quick estimates by runninggrep -ri "let " empress/support_files/js/* | wc -l
, etc. and found that there are 412var
s, 9let
s, and 4const
s in the Empress JS codebase. We should prooobably just make everythingvar
.Use foreach instead of for loops? Where possible, and where it wouldn't be an efficiency decrease. (
Map
s can apparently be directly iterated over, so maybe those would pair well with this.)We may want to go about this by figuring out what browser / JS version we want to target, and then... adjusting our build setup accordingly? Maybe by adjusting jshint, or adding Babel somewhere to verify that we can use these things and still target all the environments we want to support. I'm not really sure what the best practice would be (I suspect it'd involve a lot of messing around with adding webpack or rollup to the project).
Thanks @dhakim87, @wasade, and @qiyunzhu for bringing up these points respectively during code review today!
Edit: we should also update the documentation to mention things like browser requirements, etc.
The text was updated successfully, but these errors were encountered: