var => let/const #452

elboletaire · 2017-04-07T21:37:17Z

Closes #436

Note that #450 must be merged first (this branch has been created from that PR branch).

elboletaire · 2017-04-07T22:01:23Z

hhmmm.. it seems that in 87d3cde eslint also removed the double negations 😕

All the tests are passing, but I've added the WIP tag until:

You confirm me to undo that double negation changes.
You merge Replace jshint by eslint #450 first, as this PR depends on that one (which will reduce the amount of files in this PR diff).

…' into f/var-let-const-436

matteofigus · 2017-04-07T22:05:13Z

Hi, If you are happy and all is green, feel free to remove the WIP tag and leave it to me. I am in favour of removing the unnecessary double negations and I believe eslint tried to do that: for some I see it removed them, for some I still see them - but I definitely will spend some time to check them one by one. In case it turns out to be too much of a work, I can still make a separate PR for that and then we can merge this later so that it has less moving parts 👍

elboletaire · 2017-04-07T22:06:28Z

Removed WIP tag then 😃

I'm creating another PR setting indent to be 2 spaces (it's off now, due to a lot of tabs found on the code, combined with spaces).

nickbalestra · 2017-04-07T23:11:33Z

@elboletaire instead of having an eslintrc.json config why not just an eslintConfig in package.json ? I personally like that fact that nowadays you can almost put all the configs within package.json instead of spreading them over multiple dot files. What's your take on this ? cc @matteofigus

nickbalestra

Nice! Thanks @elboletaire

nickbalestra · 2017-04-07T23:13:51Z

client/src/href-builder.js

@@ -22,8 +22,8 @@ module.exports = function(config){
        throw settings.missingComponentName;
      }

-      var lang = options.headers['accept-language'],
-          forwardLang = config.forwardAcceptLanguageToClient === true;
+      const lang = options.headers['accept-language'];


❤️ so much better :)

nickbalestra · 2017-04-07T23:15:48Z

client/src/href-builder.js

-          registryUrl = clientRenderingEndpoint,
-          registrySegment = registryUrl.slice(-1) === '/' ? registryUrl : (registryUrl + '/'),
-          qs = !!component.parameters ? ('/?' + querystring.stringify(component.parameters)) : '';
+      const versionSegment = component.version ? ('/' + component.version) : '',


I personally find those multiple assignments pretty annoying, can we have a pr in the future to remove them ? 😈 😅 😃

lol yes, indeed

nickbalestra · 2017-04-07T23:16:43Z

client/src/utils/helpers.js

  each: function(obj, fn){
    if(_.isArray(obj)){
-      for(var i = 0; i < obj.length; i++){
+      for(let i = 0; i < obj.length; i++){


nickbalestra · 2017-04-07T23:17:36Z

client/src/utils/merge-objects.js

@@ -1,7 +1,7 @@
 'use strict';

 function addProperties(source, destination) {
-  for (var key in source) {
+  for (const key in source) {


interesting, I would have imagined this to be a let...

fyi http://stackoverflow.com/a/31987962/407456 :)

ah...not all loops are born equals..

elboletaire · 2017-04-07T23:49:03Z

About the eslintrc.json, if you decide to move it to package.json you should mention it in the readme, as it's not a default behavior. More over, as I said in #450, the grunt-eslint package does not provide a way (or at least I didn't find it) to specify the ignored files, so you'll still need the .eslintignore file.

Now that we speak about the files in the root folder... why that .node-version file, if I may ask? 🙄

nickbalestra · 2017-04-07T23:51:42Z

True. node-version is for avn & similar, but I guess there is a way to probably rely on engines for avn & co as well...

But yes, for now on we can keep it, and yes, now that you mention I would like if we get rid of grunt down the road, but definitely not a priority.

elboletaire · 2017-04-08T00:06:37Z

@nickbalestra casually today (well, for me it was yesterday, it's 2am here now) I've been at nodeconf (Barcelona) and I discovered this package: https://github.com/semantic-release/semantic-release which, if I'm not wrong, only relies on git and a npm registry.

matteofigus

Just 2 little changes. Everything else looks good.

matteofigus · 2017-04-08T01:25:59Z

src/registry/views/static/component-info.js

@@ -1,6 +1,6 @@
 'use strict';


This is client-side code that then gets uglified by jade import - when making this switch the uglification fails :(
Can we add a file ignore for this rule in this file please?

Sure. It's strange because there were some client files that crashed setting const and let due to the use strict, but not these two files 😕

Yeah, I realised the UI wasn't covered by any tests so I managed to discover this manually. To facilitate testing, I created a separate PR to add a couple of test: #455 - so this should not happen again in the future ;)

matteofigus · 2017-04-08T01:26:17Z

src/registry/views/static/list-components.js

@@ -1,45 +1,45 @@
 'use strict';


Same here, client side code that should stay with var for now

elboletaire · 2017-04-08T09:53:28Z

Both list-components.js and component-info.js were reverted.

…/oc into f/var-let-const-436

matteofigus · 2017-04-08T17:43:30Z

Thanks for working on this @elboletaire 👍

elboletaire added 4 commits April 7, 2017 21:52

refs opencomponents#436 Set no-var and prefer-const to 'error'

fe5ab2b

refs opencomponents#436 Run eslint --fix

87d3cde

refs opencomponents#436 Fixed oc-client

0fad4df

refs opencomponents#436 Monkey patching..

a05932c

elboletaire changed the title ~~var => let/const~~ [WIP] var => let/const Apr 7, 2017

refs opencomponents#436 Merge remote-tracking branch 'upstream/master…

782a916

…' into f/var-let-const-436

elboletaire changed the title ~~[WIP] var => let/const~~ var => let/const Apr 7, 2017

elboletaire assigned matteofigus Apr 7, 2017

nickbalestra reviewed Apr 7, 2017

View reviewed changes

nickbalestra approved these changes Apr 7, 2017

View reviewed changes

elboletaire mentioned this pull request Apr 8, 2017

Fix indentation #453

Merged

matteofigus requested changes Apr 8, 2017

View reviewed changes

refs opencomponents#436 Reverted const/let changes to browser files

52b17b2

refs opencomponents#436 Merge branch 'master' of github.com:opentable…

42a157c

…/oc into f/var-let-const-436

matteofigus approved these changes Apr 8, 2017

View reviewed changes

matteofigus merged commit b2a10d9 into opencomponents:master Apr 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

var => let/const #452

var => let/const #452

elboletaire commented Apr 7, 2017

elboletaire commented Apr 7, 2017

matteofigus commented Apr 7, 2017 •

edited

Loading

elboletaire commented Apr 7, 2017

nickbalestra commented Apr 7, 2017

nickbalestra left a comment

nickbalestra Apr 7, 2017

nickbalestra Apr 7, 2017

elboletaire Apr 7, 2017

nickbalestra Apr 7, 2017

nickbalestra Apr 7, 2017

elboletaire Apr 7, 2017

nickbalestra Apr 7, 2017

elboletaire commented Apr 7, 2017 •

edited

Loading

nickbalestra commented Apr 7, 2017 •

edited

Loading

elboletaire commented Apr 8, 2017 •

edited

Loading

matteofigus left a comment

matteofigus Apr 8, 2017

elboletaire Apr 8, 2017

elboletaire Apr 8, 2017

matteofigus Apr 8, 2017

elboletaire Apr 8, 2017

matteofigus Apr 8, 2017

elboletaire Apr 8, 2017

elboletaire commented Apr 8, 2017

matteofigus commented Apr 8, 2017

var => let/const #452

var => let/const #452

Conversation

elboletaire commented Apr 7, 2017

elboletaire commented Apr 7, 2017

matteofigus commented Apr 7, 2017 • edited Loading

elboletaire commented Apr 7, 2017

nickbalestra commented Apr 7, 2017

nickbalestra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elboletaire commented Apr 7, 2017 • edited Loading

nickbalestra commented Apr 7, 2017 • edited Loading

elboletaire commented Apr 8, 2017 • edited Loading

matteofigus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elboletaire commented Apr 8, 2017

matteofigus commented Apr 8, 2017

matteofigus commented Apr 7, 2017 •

edited

Loading

elboletaire commented Apr 7, 2017 •

edited

Loading

nickbalestra commented Apr 7, 2017 •

edited

Loading

elboletaire commented Apr 8, 2017 •

edited

Loading