Feature: Allow set specific template #171

luizdesign · 2017-08-22T19:46:33Z

Proposal to resolve issue #170

luizdesign · 2017-08-22T19:52:51Z

Working to fix broken unit test

codecov · 2017-08-22T20:37:57Z

Codecov Report

Merging #171 into master will decrease coverage by 0.48%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
- Coverage   96.59%   96.11%   -0.49%     
==========================================
  Files          13       13              
  Lines         529      540      +11     
  Branches       92       94       +2     
==========================================
+ Hits          511      519       +8     
- Misses         18       21       +3

Impacted Files	Coverage Δ
lib/fetch-template.js	`91.48% <94.11%> (-5.74%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8344613...0c8a6dc. Read the comment docs.

codecov · 2017-08-22T20:37:57Z

Codecov Report

Merging #171 into master will decrease coverage by 1.45%.
The diff coverage is 65%.

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
- Coverage   96.59%   95.14%   -1.46%     
==========================================
  Files          13       13              
  Lines         529      535       +6     
  Branches       92       94       +2     
==========================================
- Hits          511      509       -2     
- Misses         18       26       +8

Impacted Files	Coverage Δ
lib/fetch-template.js	`78.57% <65%> (-18.66%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8344613...a7c5a34. Read the comment docs.

vigneshshanmugam · 2017-08-23T12:55:57Z

package-lock.json

@@ -0,0 +1,2039 @@
+{


can you remove the package-lock file? we use yarn.lock

vigneshshanmugam · 2017-08-23T13:02:31Z

lib/fetch-template.js

-                    return parseTemplate(baseTemplate);
-                }
+        return getTemplatePath(request, templatesPath)
+            .then((templatePath) => {


can you combine it by this way

getTemplatePath(request, templatesPath) .then(readFile) .then(baseTemplate)

looks cleaner IMO.

vigneshshanmugam · 2017-08-23T13:07:55Z

Thanks a lot for working on this. If you are interested, feel free to use https://github.com/ljharb/util.promisify to promisify the functions. We thought of using that package anyways.

mroderick · 2017-08-23T13:14:28Z

lib/fetch-template.js

+ * @return {Promise} Template Path on success or TemplateError on fail
+ */
+const getTemplatePath = (request, templatesPath) => new Promise((resolve, reject) => {
+    fs.lstat(


If you're using node 8.x, you can use util.promisify to make this code simpler

I followed the code style of 'readFile' function, however we can change the both functions to promisify sintaxe. What do you think? Or do we open a new issue to discuss about it?

We can also do it in a separate PR.

Ok @vigneshshanmugam, I will open a issue about it.

mroderick · 2017-08-23T13:17:41Z

If you are interested, feel free to use https://github.com/ljharb/util.promisify to promisify the functions. We thought of using that package anyways.

@vigneshshanmugam is util.promisify not available?

vigneshshanmugam · 2017-08-23T13:19:55Z

@mroderick We still support node 6, so we can use that polyfill for node < 8

vigneshshanmugam · 2017-08-23T15:28:42Z

lib/fetch-template.js

-        const templatePath = path.join(templatesPath, pathname) + '.html';
-
-        return readFile(templatePath)
+        return getTemplatePath(request, templatesPath)


Two problems with this approach

the url parsing happens twice with this approach. In getTemplatePath when its a directory and inside baseTemplateFn.

You would also need to look at modify this logic

tailor/lib/fetch-template.js

Line 63 in 8344613

const baseTemplatePath = path.join(templatesPath, templateName) + '.html';

Will totally break if the templatesPath is a single file instead of dir.

Interesting, I will check this points as soon as possible!

@vigneshshanmugam I adjust the duplicated url parsing and refactoring the logic to factory the complete template path. Now I working for break the process in case of single file

@vigneshshanmugam Done!

Looks good to me 👍

vigneshshanmugam · 2017-08-24T21:15:55Z

lib/fetch-template.js

-                    return parseTemplate(baseTemplate);
-                }
+        return getTemplatePath(templatesPath, pathname)
+            .then((templateStat) => {


just a small change to keep the indentation depth minimal

.then({path} => readFile(path)) .then(baseTemplate => ....)

@vigneshshanmugam I understand your point of view, however i use the templateStat object to break the template parse logic on resolve the readFile promise. In your logic this not working apparently:

.then(({path}) => readFile(path)) .then(baseTemplate => { // templateStat is not defined })

We can pass the complete object to readFile, add the baseTemplate to object and resolve the promise with the object. What do you think about it?

Ahh thanks for pointing out. No this is fine then, We can do the cleanup as part of using util.promisify library changes.
lets get this merged :)

vigneshshanmugam · 2017-08-25T15:45:31Z

👍

addityasingh · 2017-08-25T15:51:54Z

👍

Luiz Kota added 2 commits August 22, 2017 13:33

Allow the templatesPath to be a file or folder

4f67bad

Using Promise to valid if template exists

76801ce

Adjusting scenario with baseTemplateFn

a7c5a34

vigneshshanmugam reviewed Aug 23, 2017

View reviewed changes

mroderick reviewed Aug 23, 2017

View reviewed changes

Luiz Kota added 2 commits August 23, 2017 10:28

Removing package-lock

20467d6

Refactoring promise handler

ee8e062

luizdesign mentioned this pull request Aug 23, 2017

To use 'Promisify' to simplify the code #172

Closed

vigneshshanmugam reviewed Aug 23, 2017

View reviewed changes

Luiz Kota added 4 commits August 23, 2017 17:27

Isolate logic to factory the complete template file path

cd8eb43

Using template string to factory the file path

31896e5

Removing duplicated url parse process

78f4270

Break the parse template cycle to a specific template file

0c8a6dc

vigneshshanmugam reviewed Aug 24, 2017

View reviewed changes

vigneshshanmugam approved these changes Aug 25, 2017

View reviewed changes

vigneshshanmugam merged commit 7d0fff2 into zalando:master Aug 25, 2017

vigneshshanmugam mentioned this pull request Aug 25, 2017

Loading a specific template insted of a folder #170

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Allow set specific template #171

Feature: Allow set specific template #171

luizdesign commented Aug 22, 2017

luizdesign commented Aug 22, 2017

codecov bot commented Aug 22, 2017 •

edited

Loading

codecov bot commented Aug 22, 2017

vigneshshanmugam Aug 23, 2017

luizdesign Aug 23, 2017

vigneshshanmugam Aug 23, 2017

luizdesign Aug 23, 2017

vigneshshanmugam commented Aug 23, 2017

mroderick Aug 23, 2017

luizdesign Aug 23, 2017

vigneshshanmugam Aug 23, 2017

luizdesign Aug 23, 2017

luizdesign Aug 23, 2017

mroderick commented Aug 23, 2017

vigneshshanmugam commented Aug 23, 2017 •

edited

Loading

vigneshshanmugam Aug 23, 2017

luizdesign Aug 23, 2017

luizdesign Aug 23, 2017

luizdesign Aug 24, 2017

vigneshshanmugam Aug 24, 2017

vigneshshanmugam Aug 24, 2017

luizdesign Aug 25, 2017

vigneshshanmugam Aug 25, 2017 •

edited

Loading

vigneshshanmugam commented Aug 25, 2017

addityasingh commented Aug 25, 2017

Feature: Allow set specific template #171

Feature: Allow set specific template #171

Conversation

luizdesign commented Aug 22, 2017

luizdesign commented Aug 22, 2017

codecov bot commented Aug 22, 2017 • edited Loading

Codecov Report

codecov bot commented Aug 22, 2017

Codecov Report

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 Aug 23, 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

mroderick commented Aug 23, 2017

vigneshshanmugam commented Aug 23, 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

vigneshshanmugam Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

vigneshshanmugam commented Aug 25, 2017

addityasingh commented Aug 25, 2017

codecov bot commented Aug 22, 2017 •

edited

Loading

vigneshshanmugam commented Aug 23, 2017 •

edited

Loading

vigneshshanmugam Aug 25, 2017 •

edited

Loading