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

feature: Add document property to owner #1740

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pedro00dk
Copy link

@pedro00dk pedro00dk commented May 24, 2023

Summary

I currently work on an application based on microfrontends (MFEs) architecture. We have several teams working on different parts of the application and we currently use React.
Our setup consists on having each MFE contained in its own shadow root, having its own style and dependencices.

All the MFEs also use a component library that is based on webcomponents. Since different versions of the library are going to be used by different teams, we can't use the default window.customElements registry to store webcomponents.
We current use this polyfill that allows us to attach custom element registries to the MFEs shadow roots. The polyfill also attaches other functions to the shadow root e.g. createElement, importNode, etc... so it can be used as if it as a Document instance.

We also use React to render custom elements from the shadowRoot rather than window.
This is possible because when rendering elements, react will use the ownerDocument of the element passed to createRoot, so we can easily set the element's ownerDocument to be the shadowRoot itself and the render phase will use the custom functions attached to the shadow root. (this is where react gets the ownerDocument, search for getOwnerDocumentFromRootContainer)

I have been trying to use Solid as an alternative to react in our application.
One thing that is preventing me from that is the fact that when rendering templates with custom elements, solid always uses document.importNode, there is no way to change it to use documents other than window.document.

So my idea was to make it possible to change the document that is used for rendering.
The idea consists on two small changes on this project (this PR), and on dom-expressions.

In this PR I added a new document property to owners when solid's createRoot and createComputation are called (not sure if it is needed on computations).
When child owners are created, I copy the document ref to make access easier.
One thing that concerns me is that the signal.js file there are no mentions to any DOM apis/types. So maybe that could be done in another way. In any way, the owner.document property is never used, just set.

In the dom-expressions PR I make two changes to the client.js file, one in the render function, it now always pass a owner to createRoot, that owner contains the document property set to the root element's ownerDocument.
And in the template function, instead of just calling document.importNode, it uses (getOwner().document || document).importNode.

How did you test this change?

This is my first PR in solid, I did not find much information on how to create a test build for both projects at once.
Since the amount of changes is very small, made the proposed changes directly to the solid dist files in my node_modules directory to verify that it works.

All existing tests on both projects also passed, I'm not sure if a test is necessary on this PR, it is just a new optional property on Owner. Maybe on the dom-extensions PR, but again, minor changes to render and template function, which I think are being tested already.

@changeset-bot
Copy link

changeset-bot bot commented May 24, 2023

⚠️ No Changeset found

Latest commit: 1b45169

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ryansolid
Copy link
Member

ryansolid commented May 24, 2023

I have to think about this. The core reactive library is platform agnostic. It isn't aware of the DOM. So patching owner doesn't feel right. But I'm not sure how else to get that in as I haven't thought about it.

@pedro00dk
Copy link
Author

pedro00dk commented May 24, 2023

Another implementation suggestion is to replace document in Owner by a reference to the root owner or any unique object/symbol that can be an index in a weakmap.
Then, on dom-expressions, a weakmap whose keys are the root owner (or symbol) and values the elements' ownerDocuments
setup in the render function, could be used to get the correct ownerDocument in the template function.
That makes signals.js platform agnostic again.

The root property can be used to store referentially unique object accessible in the owner tree. This property is copied to child owners and computations.
@pedro00dk pedro00dk force-pushed the feat-dynamic-document branch from 600298f to 1b45169 Compare May 25, 2023 00:08
@ryansolid
Copy link
Member

This is better.. only thing I want to check now is that this doesn't tie our hands on the Solid 2.0 reactivity design. I don't know if we want to look roots through like this. It is probably harmless, but I don't want to depend on something that we may change.

@pedro00dk
Copy link
Author

A third option is to add some sort of owner tree identifier, just a number or string, and pass it down like what is being done to the root owner. And use an object or map instead of the weakmap, we can cleanup the object entry in the dispose function.

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.

2 participants