Extract tag handling from request handling #173

DeTeam · 2017-08-25T14:13:18Z

Motivation

Right now in tailor request-handler.js contains quite a lot of logic. It's responsible for several things:

Kicking of context and template fetching
Assembling of the tag-handling logic
Deciding on how to handle various events in the app (both in the result stream and in the primary fragment)
Takes care of async stream management

With so many responsibilities it's getting harder to allow more flexible composition to users of tailor. One particular use-case: we want to introduce a new tag, which itself would result into some template which has to be processed in the similar way. handleTag function already allows returning streams, but it's quite an overhead to duplicate existing handling algorithm.

Solution: extract tag/template handling logic and make it composable. Right now it's extracted under buildBlock which can be used inside your own implementation of handleTag, so we allow recursion.

Things which were standing in the way during implementation:

index management and asyncStream
shouldWriteHead was kind of everywhere
request object is all over the place + kinda need to separate tailor context (the one overriding fragment info) and more broad term context (some state which is being modified during template processing, for instance, index)

TODOs:

Separate tag-handling logic (working name - "block", though I don't really like it, mb we can come up with smth better)
Add support for per-fragments contexts (this is not directly related, so we might consider make a separate PR with it)
Make sure parsing template within buidBlock works properly
Extract and revert example changes
Replace package lock with yarn lock
Complete the unit tests
Check performance impact
Adjust the documentation

DeTeam · 2017-08-25T14:14:12Z

👎

DeTeam · 2017-08-25T15:58:00Z

examples/custom-tags/templates/index.html

@@ -0,0 +1,4 @@
+<script type="text/javascript"></script>


Without it, the pipe script would be inserted after the switcher and won't be available to it's content. @vigneshshanmugam mb you know the nice way to do it?

Yeah pipe is inserted before fragment tag or one of the tags in handleTags list. Just pass them in the example :)

~~You mean pipeTags?~~ (it's internal)

Nevermind :)

vigneshshanmugam · 2017-08-25T16:05:38Z

package-lock.json

@@ -0,0 +1,2754 @@
+{
+  "name": "node-tailor",


we use yarn.lock, can you update yarn.lock instead?

Yes, will do!

There were no updates, I just happened to use latset npm & node %)

DeTeam · 2017-08-25T16:07:34Z

index.js

@@ -64,7 +64,7 @@ module.exports = class Tailor extends EventEmitter {

        requestOptions.parseTemplate = parseTemplate(
            [requestOptions.fragmentTag].concat(requestOptions.handledTags),
-            ['script', requestOptions.fragmentTag]
+            ['script', requestOptions.fragmentTag].concat(requestOptions.handledTags)


@vigneshshanmugam I guess you were meant this. Those custom handledTags where not passed to pipe tags, so for my switcher example it was not added. Not sure it's the best way, though.

Yeah you are right. Just feels like this gives too many options of having where pipe should be inserted. Actually, pipe is never required if its not a fragment, "script" was added because anyone can add fragment in the head like <script src="blah" type="fragment"/>.

I would move the fragment in your example before switcher to make things easier.

Or have a script tag :)
It was an interesting side-effect I didn't think about.
People who implement their own tags should kinda be aware then :)

Yeah true, lets remove these options and insert a empty div tag like before.

vigneshshanmugam · 2017-08-28T09:51:32Z

examples/custom-tags/index.js

+
+// Fragment server - Any http server that can serve fragments
+http.createServer((req, res) => {
+    const urlObj = require('url').parse(req.url, true);


can you move the require to the top.

vigneshshanmugam · 2017-08-28T13:18:56Z

lib/request-handler.js

+
+    Promise.all([
+        templatePromise,
+        contextPromise


an error in context should not control the page and return 500.

There's a catch above.. Not super visible, though %)

cool. missed it :)

vigneshshanmugam · 2017-08-28T13:34:06Z

lib/block.js

+
+    const resultStream = new StringifierStream((tag) => {
+
+        const {


this destructuring logic happens on every invocation of tag, should be moved out of of this function

vigneshshanmugam · 2017-08-28T13:37:30Z

lib/block.js

+                    }
+                };
+
+                fetchDynamicContext(tag)


IMO this creates unnecessary complexity in tailor itself and can be handled outside in requestFragment logic.

Tailor itself is extendable, we may not need this feature here.

Thoughts? @grassator @mo-gr

Partially agreed :) It doesn't really belong to this PR, moving out.

vigneshshanmugam · 2017-08-28T14:23:04Z

lib/request-handler.js

+
+            fragment.on('fallback', (err) => {
+                this.emit('error', request, err);
+                // TODO: add some responseHeaders?


why not responseHeaders like before? Do you have anything specific in mind?

Its also configurable through filterResponseHeaders function

vigneshshanmugam · 2017-08-28T14:23:12Z

lib/request-handler.js

+
+            fragment.on('error', (err) => {
+                this.emit('error', request, err);
+                // TODO: add some responseHeaders?


vigneshshanmugam · 2017-08-28T14:26:24Z

lib/request-handler.js

+        }, options, context));
+
+
+        FRAGMENT_EVENTS.forEach((eventName) => {


assuming these events are for fragments. Could you explain me why it is required here?

It's part of the tailor API (fragment:start, fragment:end, etc. events.) for fragments. It was there before (almost exact same code), difference is that now instead of subscribing to the fragment directly we subscribe to the "block" events (it propagates them), cause fragments are not directly exposed to the request handler.

Does this make sense to you?

Yeah cool, looks a bit bad that we need to handle it twice.

vigneshshanmugam · 2017-08-28T14:27:41Z

tests/serializer.js

@@ -9,6 +9,8 @@ describe('Serializer', () => {
    const serializerOptions = {
        treeAdapter: adapter,
        slotMap: new Map(),
+        addPlaceholders: true,


don't think its used anywhere.

vigneshshanmugam · 2017-08-28T14:32:39Z

lib/block.js

+            return pipeDefinition(pipeInstanceName);
+        }
+
+        if (asyncStream && placeholder === 'async') {


don't think we need this asyncstream check. Its gonna be present all the time.

As an enhancement we can do this

create asyncstream and placeholders only when async fragments are present in the template.

vigneshshanmugam · 2017-08-31T09:56:25Z

lib/block.js

+        handleTag,
+        requestFragment,
+
+        // Should the two below be one???


can you explain what you mean here? having a single option instead of 3 separate options?

Yeah, it's a note to myself: we have many pipe-related options, I was wondering if some of the options can be joined (the function part with the attribute part, cause we just pass one to another :D )

vigneshshanmugam · 2017-08-31T10:02:26Z

lib/block.js

+                });
+            });
+
+            const isAsync = fragment.attributes.async;


shall we keep the code similar to how it was in request-handler?

tailor/lib/request-handler.js

Line 105 in 7d0fff2

const { attributes: { async, primary }, stream } = fragment;

Agreed!

Wow, how can you comment with code snippets like this? oO

hehe.. there is a small option at the left end where you can copy the preview link once you click on the line.

vigneshshanmugam · 2017-08-31T10:08:10Z

lib/block.js

+        try {
+            result = handleTag(request, context, tag);
+        } catch (error) {
+            console.error(error);


should we handle the error here? isn't already wrapped in request-handler in catch statement

I think we can remove the try-catch. If this throw, error event should be emitted on the stringifier stream and get caught in the request-handler.

vigneshshanmugam · 2017-08-31T10:09:26Z

lib/block.js

+        if (name === 'fragment') {
+            // Freeze current context & update the indexes
+            const fragmentContext = context;
+            context = Object.assign({}, context, {


Since the only changing part here is index, can we have a just update index instead of doing Object overrides.

Actually, not sure here, I pass the context which was there at the time we found the tag further down the line. If I mutate it, fragment might be having some incorrect data inside and then I'd mess up indexes :(

vigneshshanmugam · 2017-08-31T10:09:56Z

lib/block.js

+            result.on('fragment:found', () => {
+                resultStream.emit('fragment:found');
+
+                context = Object.assign({}, context, {


vigneshshanmugam · 2017-08-31T10:11:38Z

lib/request-handler.js

+        contextPromise
+    ]).then(([ parsedTemplate, context ]) => {
+        const blockStream = buildBlock(request, Object.assign({
+            index: 0,


don't allow useland code to update index at any point.

Wow, you're right. Thanks for catching!

vigneshshanmugam · 2017-08-31T10:26:12Z

perf/benchmark.js

+const handleTag = (request, context, tag) => {
+    if (tag && tag.name === 'switcher') {
+        const stream = buildBlock(request, context);
+        process.nextTick(() => {


is it required? buildBlock a sync function

I was thinking about kinda real-time example when data is being pushed into this "block" sometime later on (but without big impact on performance).

Hmmm true, then lets introduce artificial delay and check if back pressure is handled properly.. setTimeout would suffice.

DeTeam · 2017-09-01T08:43:34Z

Also, let's rename buildBlock to something like:

processTemplate
handleTemplate
other options?

* template should be parsed into a set of Buffers + Objects

vigneshshanmugam · 2017-09-01T12:43:44Z

~~parseTemplate sounds good to me.~~(processTemplate)

DeTeam · 2017-09-01T12:45:14Z

@vigneshshanmugam we already have parse coming from parse5, it's a bit different, I'd stick to processTemplate

DeTeam · 2017-09-01T12:55:11Z

Rename is done, processTemplate is there!

DeTeam · 2017-09-01T12:58:45Z

Now I also need to check if it make sense to split context and options again

* Dynamic context removed (got carried from another experiment) * Destructive assignment in block.js moved one level above to not be called on every tag * Tests adjusted

* responseHeaders are used in the way it was used before * remove unused parameter (addPlaceholders) from tests * don't check for async (we assume it's always there)

* No new context obj created, mutate index field in te old one * Clean up commends * Align property names with the way they are in request handler * don't allow overriding index field

DeTeam · 2017-09-04T11:51:07Z

Rebased. No conflicts. Now going to apply options-context changes.

vigneshshanmugam · 2017-09-04T12:54:56Z

tests/handle-tag.js

@@ -42,7 +42,7 @@ describe('Handle tag', () => {
        mockHandleTag.returns('');
        http.get('http://localhost:8080/template', (response) => {
            const request = mockHandleTag.args[0][0];
-            const tag = mockHandleTag.args[0][1];
+            const tag = mockHandleTag.args[0][3];


can we keep the tag as arg1 .. otherwise it wont be backwards compatible.

aaaaah
good point
yes

vigneshshanmugam · 2017-09-06T14:52:04Z

tests/process-template.js

+            resultStream.write({ placeholder: 'async' });
+            resultStream.end();
+        });
+        it('insert pipe definition in the beginning');


once we finish the tests for process-template we can go ahead and merge it.. rest looks good to me..

Also seem like we're not changing the API much here. Mb minor version update would be suitable.

codecov · 2017-09-06T15:52:58Z

Codecov Report

Merging #173 into master will decrease coverage by 0.04%.
The diff coverage is 96.03%.

@@            Coverage Diff             @@
##           master     #173      +/-   ##
==========================================
- Coverage   96.66%   96.61%   -0.05%     
==========================================
  Files          13       14       +1     
  Lines         539      561      +22     
  Branches       93       95       +2     
==========================================
+ Hits          521      542      +21     
- Misses         18       19       +1

Impacted Files	Coverage Δ
lib/transform.js	`100% <100%> (ø)`	⬆️
lib/serializer.js	`95.87% <100%> (+0.04%)`	⬆️
lib/process-template.js	`100% <100%> (ø)`
lib/parse-template.js	`100% <100%> (ø)`	⬆️
lib/request-handler.js	`95.04% <93.65%> (-1.11%)`	⬇️

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 0c7de02...10a73dd. Read the comment docs.

vigneshshanmugam · 2017-09-06T16:12:27Z

tests/process-template.js

+                const doesMatch = r.test(data);
+                let message = `No match for the fragmend(index: ${index}, text: ${fragmentText})`;
+                if (!doesMatch) {
+                    console.log(r, data);


should we throw error here instead?

vigneshshanmugam · 2017-09-06T16:16:13Z

👍

DeTeam · 2017-09-06T16:16:37Z

👍

DeTeam force-pushed the blocks branch from 96ff001 to 3ed6dd8 Compare August 25, 2017 14:16

DeTeam commented Aug 25, 2017

View reviewed changes

vigneshshanmugam reviewed Aug 25, 2017

View reviewed changes

DeTeam commented Aug 25, 2017

View reviewed changes

vigneshshanmugam reviewed Aug 28, 2017

View reviewed changes

DeTeam force-pushed the blocks branch from 28b7fea to 123d99e Compare August 28, 2017 14:16

vigneshshanmugam reviewed Aug 28, 2017

View reviewed changes

DeTeam force-pushed the blocks branch from e0009c9 to 2155c7e Compare August 29, 2017 15:06

vigneshshanmugam reviewed Aug 31, 2017

View reviewed changes

Timur Amirov added 2 commits September 4, 2017 13:50

Add dynamic context functionality (+some rought tests)

9fd4671

Tag handling extracted

233122e

Timur Amirov added 6 commits September 4, 2017 13:50

Remove package-lock.json file

1bfd826

Don't extend pipe tags, manually insert script instead

aedfca9

Clean up after CR

c318887

* Dynamic context removed (got carried from another experiment) * Destructive assignment in block.js moved one level above to not be called on every tag * Tests adjusted

Address comments

ea9458f

* responseHeaders are used in the way it was used before * remove unused parameter (addPlaceholders) from tests * don't check for async (we assume it's always there)

Address PR comments

6ba7438

* No new context obj created, mutate index field in te old one * Clean up commends * Align property names with the way they are in request handler * don't allow overriding index field

Rename all the things

67c272d

DeTeam force-pushed the blocks branch from 494ce9b to 67c272d Compare September 4, 2017 11:50

Separate options from context (context has its very own meaning)

5ad73d5

vigneshshanmugam reviewed Sep 4, 2017

View reviewed changes

Make sure to use fragment tag name from options for comparison

8a062ae

DeTeam force-pushed the blocks branch from 19361f9 to 8a062ae Compare September 4, 2017 16:41

Timur Amirov and others added 6 commits September 5, 2017 15:28

Make handleTag signature more compatible with the old one

ef9e1e2

small refactor

e6b8597

Extract index management to a stand-alone function

958b50f

Less event listeners/emits (subscribe only on fragment:found)

97d95d7

Event listeners related changes

cabd907

serializer - few changes and add tests

384147b

vigneshshanmugam reviewed Sep 6, 2017

View reviewed changes

zalando deleted a comment from codecov bot Sep 6, 2017

Add some tests for processTemplate

f3eb555

Typo fix

74ec753

vigneshshanmugam reviewed Sep 6, 2017

View reviewed changes

Update the README + remove some unused logging

10a73dd

vigneshshanmugam approved these changes Sep 6, 2017

View reviewed changes

vigneshshanmugam merged commit 99ba67e into zalando:master Sep 6, 2017

vigneshshanmugam deleted the blocks branch September 6, 2017 16:20


		const resultStream = new StringifierStream((tag) => {

		const {

		}, options, context));


		FRAGMENT_EVENTS.forEach((eventName) => {

Extract tag handling from request handling #173

Extract tag handling from request handling #173

Conversation

DeTeam commented Aug 25, 2017 • edited by vigneshshanmugam Loading

DeTeam commented Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DeTeam Aug 25, 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

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 28, 2017 • edited Loading

Choose a reason for hiding this comment

DeTeam Aug 28, 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

DeTeam Aug 29, 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

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 31, 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

Choose a reason for hiding this comment

DeTeam commented Sep 1, 2017

vigneshshanmugam commented Sep 1, 2017 • edited Loading

DeTeam commented Sep 1, 2017

DeTeam commented Sep 1, 2017

DeTeam commented Sep 1, 2017

DeTeam commented Sep 4, 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

codecov bot commented Sep 6, 2017 • edited Loading

Codecov Report

vigneshshanmugam Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

vigneshshanmugam commented Sep 6, 2017

DeTeam commented Sep 6, 2017

DeTeam commented Aug 25, 2017 •

edited by vigneshshanmugam

Loading

DeTeam commented Aug 25, 2017 •

edited

Loading

DeTeam Aug 25, 2017 •

edited

Loading

vigneshshanmugam Aug 28, 2017 •

edited

Loading

DeTeam Aug 28, 2017 •

edited

Loading

DeTeam Aug 29, 2017 •

edited

Loading

vigneshshanmugam Aug 31, 2017 •

edited

Loading

vigneshshanmugam commented Sep 1, 2017 •

edited

Loading

codecov bot commented Sep 6, 2017 •

edited

Loading

vigneshshanmugam Sep 6, 2017 •

edited

Loading