Update Tailor using ES6 features(fixes #102) #109

addityasingh · 2017-01-16T14:03:43Z

Use ES6 de-structuring wherever applicable
Use multiline string templates using ` `
Use () => {} and remove verbose code, wherever applicable
Use Array methods like .includes(), wherever applicable
Update Tailor to use node-7.x

…into update-es6-features

vigneshshanmugam · 2017-01-16T14:08:39Z

.travis.yml

@@ -1,8 +1,7 @@
 sudo: false
 language: node_js
 node_js:
- '4.4.4'
- '6.0.0'


keep 6 as well.. We shall drop 4 dependency alone.

vigneshshanmugam · 2017-01-16T14:09:09Z

index.js

+
+const stripUrl = fileUrl => path.normalize(fileUrl.replace('file://', ''));
+const getPipeAttributes = attributes => {
+    const { primary, id} = attributes;


space after id for consistency.

vigneshshanmugam · 2017-01-16T14:45:11Z

lib/fetch-template.js

        fs.readFile(path, 'utf-8', (err, data) => {
            if (err) {
                reject(new TemplateError(err));
-                return;
+            } else {


else not needed here..

vigneshshanmugam · 2017-01-16T14:46:11Z

lib/fetch-template.js

-module.exports = function fetchTemplate (templatesPath, baseTemplateFn) {
-    return (request, parseTemplate) => {
+module.exports = (templatesPath, baseTemplateFn) => 
+    (request, parseTemplate) => {


would keep the consistency of keeping parenthesis everywhere.. keeping some function without parenthesis and others with it doesn't look good.

I am keeping parenthesis only when returning object. Since , this function returns a promise there is no need of keeping it. Thoughts?

keeping parentheses for args looks better to me and make you feel its a function.. Its just my thoughts though... If we keep it here,I would also keep it in all arrow functions to be consistent.

vigneshshanmugam · 2017-01-16T14:57:33Z

lib/serializer.js

-        const attribs = node.attribs;
-        return node.name === 'script' &&
-            attribs && attribs.type === 'slot';
+        const { attribs, name} = node; 


space after name

not sure if eslint rule for this exist but would be cool to add this rule.

@vigneshshanmugam We can use http://eslint.org/docs/rules/object-curly-spacing for this but the problem is the existing code is not consistent for any of the never or always rules. It throws 58 and 60 errors respectively with never and always rules. Which one do you think should be used across? I am planning to add this and make it consistent

vigneshshanmugam · 2017-01-16T15:06:19Z

package.json

@@ -24,7 +24,7 @@
    "test"
  ],
  "engines": {
-    "node": ">4.4.3"
+    "node": ">7.0.0"


vigneshshanmugam · 2017-01-16T15:16:57Z

lib/transform.js

+                const slotNodes = slotMap.get(slot) || [];
+                const updatedSlotNodes = [...slotNodes, node];
+                slotMap.set(slot, updatedSlotNodes);
+                this._pushText(node.next, updatedSlotNodes);


dont think this works though. updatedSlotNodes doesnt have reference to the slotMap and would not update the text nodes.. I think it should be slotMap.get(slot)

I think updatedSlotNodes contains the result of line 59-63 which is what is getting used as second parameter of this._pushText. So,

[...slotNodes, node] ≈ (slotMap.get(attribs.slot).push(node); || slotMap.set(attribs.slot, [node]);)

This is just our understanding of the syntax and seems same in functionality. But, you can confirm if the functionality is broken or not?

I am 99% sure its broken, I will confirm you anyways :) I may be wrong as well.

I am wrong.. Its working as expected.. sorry for that! :)

Cool. I was hoping to not have to change this :) @diogodadalt You were right on this one

@vigneshshanmugam @Addi90 thanks for taking the time to review it.

@diogodadalt No issues man.. I just came to know there should be a test for this as well.. Will add it after this PR has landed.

added f91bcff

vigneshshanmugam · 2017-01-16T15:18:48Z

@Addi90 Huge thanks for the PR :)

vigneshshanmugam · 2017-01-16T15:19:14Z

/cc @grassator @dazld Thoughts?

dazld · 2017-01-16T16:38:30Z

index.js

                path.join(process.cwd(), 'templates')
            ),
            fragmentTag: 'fragment',
            handledTags: [],
            handleTag: () => '',
            requestFragment,
-            pipeDefinition: (pipeInstanceName) => pipeChunk(amdLoaderUrl, pipeInstanceName),
+            pipeDefinition: pipeInstanceName => pipeChunk(amdLoaderUrl, pipeInstanceName),


how about pipeChunk.bind(null, amdLoaderUrl) ?

Yeah. We can curry a new function since pipeInstanceName is anyway the second param

dazld · 2017-01-16T16:50:40Z

lib/request-fragment.js

@@ -19,8 +19,8 @@ const requiredHeaders = {
 * @param {Object} request - HTTP request stream
 * @returns {Promise} Response from the fragment server
 */
-module.exports = function requestFragment (fragmentUrl, fragmentAttributes, request) {
-    return new Promise((resolve, reject) => {
+module.exports = (fragmentUrl, fragmentAttributes, request) =>


think this should probably stay as a named function for stack traces.

good catch @dazld

@dazld But wouldn't the stack trace use the name from the name of the import. E.g.:

const requestFragment = require('./lib/request-fragment');

I may be wrong but based on my understanding, with any error, the stack trace would use the name of the import i.e. requestFragment.

Maybe you can test it out by throwing error manually?

Irrespective of the syntax of the function, if its the default module function the error thrown is the same for me and defined by the file and line number. E.g. on throwing error in request-fragment.js, the below error is thrown with both the syntaxes:

if (response.statusCode >= 500) { reject(new Error('Internal Server Error')); } else { throw new Error('test the stack trace'); resolve(response); }

Uncaught Error: test the stack trace
at OverriddenClientRequest.fragmentRequest.on (lib/request-fragment.js:43:23)
at respond (node_modules/nock/lib/request_overrider.js:441:17)
at node_modules/nock/lib/request_overrider.js:479:11
at _combinedTickCallback (internal/process/next_tick.js:67:7)
at process._tickCallback (internal/process/next_tick.js:98:9)

alright then 👍

I have fixed the other comment from @dazld and pushed. Check if it makes sense to merge it?

awesome 👍

codecov-io · 2017-01-16T16:54:45Z

Current coverage is 91.26% (diff: 90.54%)

No coverage report found for master at 3c39e2e.

Powered by Codecov. Last update 3c39e2e...d87256a

addityasingh and others added 14 commits January 10, 2017 12:01

#99 Add codecov for integrating code coverage reports

a4b9bcb

#99 Fix badge urls in README

86e12c2

#102 Add destructuring, arrow functions to first few files

d05674f

#102 object literal shorthand and some other refactorings

8911c12

#102 Add destructuring, () => {} to request-handler.js

8390da5

#102 Add destructuring, () => {} to serializer.js

b4c14e6

Merge branch 'update-es6-features' of https://github.com/addi90/tailor …

672fdb5

…into update-es6-features

#102 Remove non-applicable TODO comment

e71e570

#102 Add parameter destructuring to make filter-headers.js more concise

4b65a9c

Merge branch 'master' into update-es6-features

0d02b0b

#102 Replace incorrect merge conflict resolution in index.js

3a3e4b9

#102 Revert parameter destructuring due to failure in node-4.x

081f8d8

#102 Revert lodash FP changes in filter-headers.js

69318dc

#102 Update Tailor to use node 7.x

e118752

vigneshshanmugam reviewed Jan 16, 2017

View reviewed changes

vigneshshanmugam changed the title ~~Update Tailor using ES6 features~~ Update Tailor using ES6 features(fixes #102) Jan 16, 2017

vigneshshanmugam requested a review from grassator January 16, 2017 16:07

dazld reviewed Jan 16, 2017

View reviewed changes

#102 Fix code comments and breaking test on node 7.x

d87256a

#102 Add curried function instead of lambda

705f70b

vigneshshanmugam approved these changes Jan 16, 2017

View reviewed changes

vigneshshanmugam merged commit 07f07dc into zalando:master Jan 16, 2017

addityasingh deleted the update-es6-features branch January 17, 2017 07:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Tailor using ES6 features(fixes #102) #109

Update Tailor using ES6 features(fixes #102) #109

addityasingh commented Jan 16, 2017

vigneshshanmugam Jan 16, 2017

vigneshshanmugam Jan 16, 2017

vigneshshanmugam Jan 16, 2017

vigneshshanmugam Jan 16, 2017

addityasingh Jan 16, 2017

vigneshshanmugam Jan 16, 2017

addityasingh Jan 16, 2017

vigneshshanmugam Jan 16, 2017

vigneshshanmugam Jan 16, 2017

addityasingh Jan 17, 2017 •

edited

Loading

vigneshshanmugam Jan 16, 2017

vigneshshanmugam Jan 16, 2017

addityasingh Jan 16, 2017

vigneshshanmugam Jan 16, 2017 •

edited

Loading

vigneshshanmugam Jan 16, 2017

addityasingh Jan 16, 2017

diogodadalt Jan 16, 2017

vigneshshanmugam Jan 16, 2017

vigneshshanmugam Jan 16, 2017

vigneshshanmugam commented Jan 16, 2017

vigneshshanmugam commented Jan 16, 2017

dazld Jan 16, 2017

addityasingh Jan 16, 2017

dazld Jan 16, 2017 •

edited

Loading

vigneshshanmugam Jan 16, 2017 •

edited

Loading

addityasingh Jan 16, 2017

vigneshshanmugam Jan 16, 2017

addityasingh Jan 16, 2017

vigneshshanmugam Jan 16, 2017

addityasingh Jan 16, 2017

vigneshshanmugam Jan 16, 2017

codecov-io commented Jan 16, 2017 •

edited

Loading

Update Tailor using ES6 features(fixes #102) #109

Update Tailor using ES6 features(fixes #102) #109

Conversation

addityasingh commented Jan 16, 2017

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

Choose a reason for hiding this comment

addityasingh Jan 17, 2017 • edited Loading

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

vigneshshanmugam Jan 16, 2017 • edited Loading

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

vigneshshanmugam commented Jan 16, 2017

vigneshshanmugam commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dazld Jan 16, 2017 • edited Loading

Choose a reason for hiding this comment

vigneshshanmugam Jan 16, 2017 • edited Loading

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

codecov-io commented Jan 16, 2017 • edited Loading

Current coverage is 91.26% (diff: 90.54%)

addityasingh Jan 17, 2017 •

edited

Loading

vigneshshanmugam Jan 16, 2017 •

edited

Loading

dazld Jan 16, 2017 •

edited

Loading

vigneshshanmugam Jan 16, 2017 •

edited

Loading

codecov-io commented Jan 16, 2017 •

edited

Loading