adobe / helix-pipeline

request | markdown | html | response
https://www.project-helix.io
Apache License 2.0
31 stars 19 forks source link

Review how context is default #474

Closed kptdobe closed 2 years ago

kptdobe commented 5 years ago

I'm not sure I like either variant. Both have side effects (modify context.request) that are hidden in the parse-markdown step. Would there be any harm in simply extracting the extension as a local const?

Originally posted by @trieloff in https://github.com/adobe/helix-pipeline/pull/473

Related code: https://github.com/adobe/helix-pipeline/blob/master/src/html/parse-markdown.js#L21-L22

parseMarkdown sets a default request object and a default extension inside this request object. This has nothing to do here and should be done before, most likely at the beginning of the pipeline.

kptdobe commented 5 years ago

Interestingly the json and xml pipeline uses the parse-mardown function thus the context.request.extension should even be false there.

My proposal is to:

@trieloff WDYT ?

The coerce default values is probably another story.

tripodsan commented 5 years ago

the context.request.extension is initialized here:

https://github.com/adobe/helix-pipeline/blob/6643dc941e56a9b8a255c416c9b86a625dfe8dfd/src/utils/openwhisk.js#L65

I think that setting it to html by default is wrong and the respective pipelines should set it accordingly. or the code that depends on it should be prepared to have it empty.

kptdobe commented 5 years ago

Yes, that's somehow what I try to say :)

Either you receive the extension as a parameter (do we really sent it in one case ?) or the extension is computed somewhere and the could should be generic for all the pipelines.

Actually, why is this part so complicated ? I think the same applies for the selector. In the pipeline code, nothing computes the selector thus I assume they are sent as a param or the property is wrong. Maybe it is just about adding the extension in the dispatch call thus in the pipeline we can assume it is correct. And the code should really cover the empty extension case instead of computing a value for it.

tripodsan commented 5 years ago

Maybe it is just about adding the extension in the dispatch call thus in the pipeline we can assume it is correct

I think the VCL code used to send those correctly.

kptdobe commented 5 years ago

ok, then the todo here would become:

I'll create the corresponding dispatch issue.

tripodsan commented 2 years ago

won't fix.