Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Podium context "invisible" getters for convenience and consistency #52

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

digitalsadhu
Copy link
Member

@digitalsadhu digitalsadhu commented Apr 2, 2020

Overview

Here is a proposal to make switching between podlet and layout accessing of the context a little more consistent. I realise that they can't entirely be the same due to the need for fetch values to be respected in some cases. (eg. public pathname) but this seemed like a possible way to avoid the confusion when accessing context values in a layout route.

What?

This PR adds an "invisible" getter property (get only, no write, non enumerable) for each parser using the registered key as the property. Since these are non enumerable, they will not show up in any serialisation.

Why?

It's confusing to users to have to access context values via a snake case string that also must include podium- at the beginning. Eg. res.locals.podium.context['podium-finn-token']

Example

layout.context.register('finnToken', new FinnTokenParser());

app.get('/', (req, res) => {
    // this is confusing, especially when it's different in the podlet
    const finnToken = res.locals.podium.context['podium-finn-token'];

   // this PR makes it possible to do this
    const finnToken = res.locals.podium.context.finnToken;

  // but the context object will not show the getters
  const keys = Object.keys(res.locals.podium.context);

  /* keys:
  [
        'podium-public-pathname',
        'podium-mount-pathname',
        'podium-mount-origin',
        'podium-requested-by',
        'podium-device-type',
        'podium-locale',
        'podium-debug',
        'podium-finn-token',
    ]
    */
});

Caveat

If a parser is a function, the getter will still return the function, it will not call the function so there is still room for some inconsistency between the layout and the podlet.

@digitalsadhu digitalsadhu requested a review from trygve-lie April 2, 2020 03:34
@digitalsadhu digitalsadhu changed the title add Podium context invisible getters for convenience and consistency add Podium context "invisible" getters for convenience and consistency Apr 2, 2020
@trygve-lie
Copy link
Contributor

Yeah.. I totally see the need for this but I think we need to think a little bit further. Initially the context was not meant to be accessed in a layout server but the reality is that there is a need for accessing it in a layout also.

We also have a second need; defining logic during de-serialization. Currently the context let you define a logic for paring something into a context value (run in a layout), but we do have cases where we want to append logic on a context value upon de-serialization (run in a podlet). One example of such is handling jwt token (which is a custom parser internally at FINN) where we in the Layout create a jwt token which is put on the context but in the podlet this needs to be unpacked to be useful. Currently all podlets now need to do that unpacking them self and its being done in different ways. It would be very nice if this unpacking could be part of the context de-serialization process.

So, I would like a little bit larger rewrite of the context where we do as follow:

  • The context module should be used in @podium/podlet to de-serialize context values.
  • A context parser should in addition to a .parse() method, have a .unpack() (suggestions for a better name is welcome) method. The .parse() method is used to make a context value in @podium/layout. The .unpack() method is used to transform a context value (http header value) into something else in @podium/podlet.
  • Upon register of a context parser in the context module, the property of the context on the context object on HttpIncoming.context should be defined. This so we create these in a static way instead of dynamically on each request.

It would also be very nice if we could make it so that we differentiate the built in context values in Podium from the custom context properties. Ideally it would be nice if a podlet could detect that on the request to a podlet there are custom context properties but the podlet is missing the required context parser to deal with it.

@digitalsadhu
Copy link
Member Author

👍 sounds good.

@leftieFriele leftieFriele self-requested a review December 4, 2023 07:40
Copy link

@leftieFriele leftieFriele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this, as the difference is confusing when starting out with Podium, this will help moving between podlet and layout context easier 💯 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants