feat(pipe): uniform steps (#377)

[rofe]

Fixes #377

Note that I slightly deviated from the proposed implementation in #377 in two places:

• I kept the once() method for now, mainly because of the special handling of the customer-specified continuation function as potential bearer of custom extensions. It felt cumbersome to check for extensions on every function that gets passed to use(). Maybe there's a simple and elegant way of doing that, but I didn't find one yet.
• I didn't implement a pipeline.replace() function, because it would violate the "seal" concept applied in this.attach(). Instead I went for a this.attach.replace() sub function and a check for a replace export on the continuation function.

[codecov]

Codecov Report

Merging #440 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #440   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          50     50           
  Lines        1149   1141    -8     
=====================================
- Hits         1149   1141    -8

Impacted Files	Coverage Δ
src/defaults/json.pipe.js	100% <ø> (ø)	⬆️
src/defaults/xml.pipe.js	100% <ø> (ø)	⬆️
src/defaults/default.js	100% <100%> (ø)	⬆️
src/defaults/html.pipe.js	100% <100%> (ø)	⬆️
src/pipeline.js	100% <100%> (ø)	⬆️

---

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 b7fb224...5f5a5c5. Read the comment docs.

[tripodsan]

• I kept the once() method for now, mainly because of the special handling of the customer-specified continuation function as potential bearer of custom extensions.

I don't understand. that's exactly the goal of #377 to get rid of special meaning of certain steps.

[tripodsan]

• I didn't implement a pipeline.replace() function, because it would violate the "seal" concept applied in this.attach().

you don't need to actually replace the function, just invoke the replacement instead of the existing one. looking at the code, this is what you do ;-)

[tripodsan]

the goal of #377 is to get rid of the once function. (yes, this will completely break backward compatibility). so there is no special handling of the cont function anymore. the default pipelines would just ensure to have an (noop) function exported as render.

[rofe]

so there is no special handling of the cont function anymore.

Got it. Should we also change the way developers can register functions to extension points, then? Right now we look at the properties before and after of the this._oncef where extensions are stored.

[rofe]

this will completely break backward compatibility

Ok, so no need to keep deprecated before() and after() methods around, either, I guess.

[rofe]

you don't need to actually replace the function, just invoke the replacement instead of the existing one. looking at the code, this is what you do ;-)

I'm not sure I understand... yes, we could keep a separate list of "overrides" and execute those instead of the original functions. But wouldn't that defeat the purpose of sealing the pipeline? Shouldn't we treat custom replacements the same way as before/after hooks?

[tripodsan]

Shouldn't we treat custom replacements the same way as before/after hooks?

yes. your comment in the description was not clear enough - but then I looked at the code, and I think it is correct.

[tripodsan]

so there is no special handling of the cont function anymore.

Got it. Should we also change the way developers can register functions to extension points, then? Right now we look at the properties before and after of the this._oncef where extensions are stored.

hmm, true.. I guess any use() function can add extension points ;-)

the problem is, that we want to be able to extend the pipe in the pre.js )(which is currently turned into the once function). if we don't have a dedicated once function anymore, we can either:

1. allow any use() function to add and extension point (which would be kind of confusing)
2. move the extend logic to the helix-cli, where we instrument the pipe.

[rofe]

so there is no special handling of the cont function anymore.

Got it. Should we also change the way developers can register functions to extension points, then? Right now we look at the properties before and after of the this._oncef where extensions are stored.

hmm, true.. I guess any use() function can add extension points ;-)

the problem is, that we want to be able to extend the pipe in the pre.js )(which is currently turned into the once function). if we don't have a dedicated once function anymore, we can either:

1. allow any use() function to add and extension point (which would be kind of confusing)
2. move the extend logic to the helix-cli, where we instrument the pipe.

I would stay away from (1) and postpone (2). For now, I think I'd keep the once() function, but maybe rename it to cont() or ext() to better illustrate its purpose.

@trieloff what's your take here?

[trieloff]

1. allow any use() function to add and extension point (which would be kind of confusing)

cool, but confusing, indeed.

2. move the extend logic to the helix-cli, where we instrument the pipe.

I don't like that.

My suggestion would be 1*:

*: we allow the use of arbitrary extensions in use(), but discourage it, e.g. by discovering when before or after are used in the second or later invocation of use() and either warning or throwing an error.

[tripodsan]

My suggestion would be 1*:

IMO there are 2 different usage patters or API's here. one is the pipeline API itself. it currently offers a Pipeline.attach.before() and Pipeline.attach.after() methods. they have no effect, once the pipeline steps are sealed. I think the signature is a bit weird, but I don't have a better option right now.

the other usage is what we offer in the defaults, eg the

const htmlpipe = (cont, context, action) => {
...
  pipe
...
    .before(sanitize).when(paranoid)
    .once(cont)
    .after(type('text/html'))
...
  action.logger.log('debug', 'Running HTML pipeline');
  return pipe.run(context);
};

the cont here, is a more or less arbitrary decision to have a designated function for the render step. also, the function also runs the pipe with the given context.

instead I propose that the default html pipe only returns the pipeline itself, but the user can then extend and run the pipe.

eg:

cont pipe = require('defaults/html.pipe.js').create();
pipe.attach.before('render', preFn);
pipe.attach.replace('render', htlRenderer);
pipe.run(context);

in order to simplify the attaching, we can have a

pipe.attach(/* options */); 

which takes an object with before, replace, after configs, like in the current pre.js. so we can still attach the extension defined in the pre.js:

cont pipe = require('defaults/html.pipe.js').create();
pipe.attach(preFn);
pipe.attach.before('render', preFn);
pipe.attach.replace('render', htlRenderer);
pipe.run(context);

[rofe]

From the implementation, it doesn't look like the pipe.attach.*() methods were ever meant to be public API. They are rather an implementation detail for one-off processing of the before and after extension objects on the cont function. This is also confirmed by the fact that the only officially documented way to extend the pipeline is to export before and after objects in html.pre.js.

The second part of this issue was about introducing a way to also replace pipeline steps. Sure, we could now also introduce an officially public pipe.attach(obj) method. But do we have known use cases so far where customers create their own pipelines and interact with the pipe object?

The more I think about it, the more I believe that the proposed changes to the extension mechanism would best be moved into a separate issue, and this issue should stay focused on uniforming pipeline steps: pipe.use() instead of pipe.before(), pipe.after() and pipe.once(). Wdyt?

[tripodsan]

The more I think about it, the more I believe that the proposed changes to the extension mechanism would best be moved into a separate issue,

well, just renaming before/after is not the main focus of the API change. rather that the cont / once() function should not have a special meaning anymore.

so the changes would be:

pipeline

• add use() method
• deprecate before(), after() and once() and just invoke use()
• add attach.replace()
• create new attach(options) method that takes a before/after/replace object.
• change all default pipelines:
  • remove all cont functions
  • do not run pipeline, just create it

helix-cli

• handle attach logic (i.e. call pipe.attach(preFn) )
• run pipeline explicitly

[rofe]

ok, i basically have the changes in the pipeline ready:

• once(), before() and after() replaced by use()
• first function with extension points wins, use() throws error on next
• attach() no longer seals the pipeline so it can be called from outside

will create a related issue for the helix-cli changes

[tripodsan]

well, ok...let's do this in 2 steps.

this PR will just add uniform steps, but leaves the defaults untouched...

then we create an additional issue that removes the run() from the default pipelines.

[rofe]

If we remove all cont functions from the default pipelines, how would helix-cli know where to insert the preFn?

[tripodsan]

If we remove all cont functions from the default pipelines, how would helix-cli know where to insert the preFn?

because we have a default (nop) step called render and can replace it.

[rofe]

I see. Let's create a separate issue for this change.

[rofe]

adobe/helix-cli#1125 is implemented in helix-cli, adding support replacing pipeline steps. With that this PR addresses all original points of #377 (uniform steps with use() and add replace support).

@trieloff @tripodsan any additional concerns with merging?

[adobe-bot]

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀