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

RFC PR: add a DSD based isolation layout to the podlet #409

Closed
wants to merge 1 commit into from

Conversation

digitalsadhu
Copy link
Member

RFC - PR - DSD based isolation in podlets

I wanted to get the ball rolling on this feature and so I've spent a fair amount of time noodling with the API surface and various challenges/constraints and I think what I have here is good enough to use as a starting point for discussion.

I've commented the code changes heavily to try to explain my thinking with the way I've set things up. What do we think about the approach? Are there glaring holes in my reasoning/approach?

*
* Eg.
* app.get("/", (req, res) => {
* res.podiumSend(res.podiumIsolate("<div>content to render</div>"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* res.podiumSend(res.podiumIsolate("<div>content to render</div>"));
* res.podiumSend("<div>content to render</div>", this.isolate);

If we in Express add a parameter to .podiumSend which is a wrapper / isolate function I think that can make for a nice way of doing this.

It also makes it possible to implement something similar for Fastify where you can add a config option to the route(s) that is the wrapper function and make it happen in a onSend hook.

fastify.addHook('onSend', (request, reply, payload) => {
 if (reply.context.config.podlet.isolate) {
  return reply.context.config.podlet.isolate(payload);
 }
})
fastify.get("/", { config: { podlet: { wrapper: this.isolate } } }, async (req, reply) => {
 reply.podiumSend("<div>...</div>");
 return reply;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Right now the arguments to podiumSend get passed on to the view render function. Changing that would be a breaking change and force us to redesign how we pass arguments to the render function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I wasn't aware of that. The docs on .render argument args is a bit vague "additional args depending on the template and what values it accepts".
🤔 the .render method isn't documented either.

Copy link
Member Author

@digitalsadhu digitalsadhu Aug 15, 2024

Choose a reason for hiding this comment

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

Yes, as I see it, we can't really touch the podiumSend API surface without a breaking change and it would be nice to avoid.

The problem with methods on the podlet, eg. podlet.render is that you will always need to pass httpIncoming. By adding an additional method on the res/reply objects, we can inject httpIncoming and nicify up the api a bit.

*
* We could consider integrating this feature into the render/podiumSend methods but this would require a rework of their method signatures to accommodate.
*
* We should consider naming of isolate. There may be better, more descriptive names? eg. podlet.wrapContent, podlet.wrapWithShadowDOM etc.
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that calling it something like wrapper or wrapContent or wrapResponseContent or similar is better than tying it specifically to the ShadowDOM.

*
* Eg.
* app.get("/", (req, res) => {
* res.podiumSend(res.podiumIsolate("<div>content to render</div>"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now the arguments to podiumSend get passed on to the view render function. Changing that would be a breaking change and force us to redesign how we pass arguments to the render function.

*
* We could consider integrating this feature into the render/podiumSend methods but this would require a rework of their method signatures to accommodate.
*
* We should consider naming of isolate. There may be better, more descriptive names? eg. podlet.wrapContent, podlet.wrapWithShadowDOM etc.
Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of like isolate since it makes the intent clear, although it kind of hides away the native platform feature. Maybe inDeclarativeShadowDOM? Verbose as heck though 🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea, it's a tricky one. I find that in general I'm struggling with naming for all the things. A part of me wants terse and a part of me wants descriptive and I can't seem to lay the matter to rest... I think descriptive maybe starting to get the upper hand...

// Ideally, when we create the DSD HTML tag for isolation, that isolation would be the podlet name to avoid the need to append or prepend naming elements.
// In order for that to be possible, the podlet name would need to follow custom element naming rules.
// An alternative strategy could be to attempt to cooerce the podlet name into a valid custom element name.
// Tricky for names that don't include a dash, underscore or camelcasing. Eg. foobar. How would we coorce that into a valid custom element name?
Copy link
Contributor

Choose a reason for hiding this comment

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

-podlet suffix (or prefix) if we don't find any - in the name?

Copy link
Member Author

Choose a reason for hiding this comment

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

True, don't love it though. It would be nice if the name used by the developer was the name in the tag in the browser if possible.

'When using isolation, podlet.name must conform to custom element naming conventions: https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry/define#valid_custom_element_names',
);

// We can leverage the new asset scope feature and add a new scope type of 'shadow-dom' to make it possible to filter out shadow dom assets for inclusion.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a platform feature or something of ours? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

scope is a feature we added fairly recently. See #355

@@ -30,6 +30,7 @@
"types": "tsc --declaration --emitDeclarationOnly && ./fixup.sh"
},
"dependencies": {
"@hide-in-shadows/html": "^0.0.5",
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless we inline it.

Suggested change
"@hide-in-shadows/html": "^0.0.5",
"@hide-in-shadows/html": "0.0.5",

@trygve-lie
Copy link
Contributor

I am wondering if this could be an argument on the constructor or as a argument on the already existing .podiumSend() method:

Constructor:

const podlet = new Podlet({
  name: 'myPodlet',
  version: '1.0.0',
  pathname: '/',
  development: true,
  isolate: false,  // Default DSD to true?
});

On .podiumSend():

res.status(200).podiumSend(`<h2>Hello world</h2>`, { isolate: false });

@trygve-lie
Copy link
Contributor

I am also unsure if we should call it isolate. What we are really doing here is turning plain html into a Declarative Shadow DOM. I think we should be more explicit on that this is what is happening. Maybe just dsd: true or something.

@digitalsadhu
Copy link
Member Author

digitalsadhu commented Aug 15, 2024

I am wondering if this could be an argument on the constructor or as a argument on the already existing .podiumSend() method:

Constructor:

const podlet = new Podlet({
  name: 'myPodlet',
  version: '1.0.0',
  pathname: '/',
  development: true,
  isolate: false,  // Default DSD to true?
});

We could certainly go for a constructor based approach.

Worth noting that I do see a use case for a separate function that the user can directly access if necessary. This function would wrap with DSD and return for the user to then do whatever they want with before they call podiumSend. We've had this as a request for our internal podlet dsd wrapping.

On .podiumSend():

res.status(200).podiumSend(`<h2>Hello world</h2>`, { isolate: false });

This would be a breaking change since arguments to podiumSend get forwarded on templates. Nice to avoid if possible.

I think the way to go in this case would be:

  1. Add internal function, call it whatever, .isolate, .dsd, etc. that returns wrapped content
  2. Add constructor arg that switches auto isolation with podiumSend on and off

Users can then flip the switch to the off position and then use the isolate/dsd function manually themselves as necessary.

@digitalsadhu
Copy link
Member Author

closing as completed in #420

@digitalsadhu digitalsadhu deleted the add_isolation_layer branch September 16, 2024 19:23
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.

4 participants