-
-
Notifications
You must be signed in to change notification settings - Fork 42
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
refactor: replace string selectors with HTMLElements for single-spa mount #106
base: main
Are you sure you want to change the base?
Conversation
Any chances that this PR could be checked? |
@@ -88,14 +88,17 @@ function mount(opts, mountedInstances, props) { | |||
} | |||
} else { | |||
domEl = appOptions.el; | |||
if (!domEl.parentNode) { | |||
throw Error( | |||
`If appOptions.el is provided to single-spa-vue, the dom element must exist in the dom. Was provided as ${appOptions.el.outerHTML}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be valid to mount to a disconnected DOM node, so I'm not sure this if statement and error are necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the reason why we added this if
is that we wanted to keep the behaviour aligned with the case where appOptions.el
is a string
.
If you think it is not necessary, I can remove it.
Let me know.
} | ||
} else { | ||
const htmlId = `single-spa-application:${props.name}`; | ||
appOptions.el = `#${CSS.escape(htmlId)}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If appOptions.el isn't provided to single-spa-vue, don't we need to add it so that Vue knows where to mount?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, appOptions.el
is defined a few lines below: https://github.com/single-spa/single-spa-vue/pull/106/files#diff-d3b89dd088913971f21ae339bd0ea76769602c03acb5d9e1efc116efe355bb28R125
The fact is from line 82 to 106 we are creating the domEl
.
But, we cannot assign it to appOptions.el
before handling the "replaceMode" behaviour.
…ount The mount function was using a string selector to mount the Vue instance. In order to add the possibility to mount inside a ShadowDom, we cannot use string selectors. So we replace the logic to be based on HTMLElements.
1ec7a40
to
3946f1f
Compare
@joeldenning I rebase the branch on main to remove the conflict. Let me know about #106 (comment) |
The Context
We are currently implementing a micro-frontend architecture using single-spa, and to avoid CSS leaks, we decided to mount our micro-frontend inside a ShadowDom.
It works fine with single-spa-angular and single-spa-react, but we have an issue with single-spa-vue.
The issue
single-spa-vue has an option called
appOptions
where you can define anel
parameter that allows us to define either an HTMLElement or a string selector which will be used to mount the Vue application.However, the parameter
el
is altered by single-spa-vue to become in either case a string selector.This string selector is used to locate the div inside the DOM to mount the Vue application, but when using a ShadowDom, it will not be able to locate the div, because it resides inside the ShadowDom.
The solution
Vue2 and Vue3 support having an HTMLElement for the
el
parameter. So instead of relying on string selectors, we could rely only on HTMLElements.Resume of the changes
The changes should be retro-compatible, however we tried to make it more robust.
If you give to the
el
parameter an element that has no parentNode (meaning it is not present in the DOM), an error will be thrown. Before nothing would happen.