Feature: Add Parcel Support [#49] #50
Conversation
swap getContainerEl with chooseDomElementGetter to emulate the React implementation and thus add Parcel support for AngularJS ✅ Closes: single-spa#49
| @@ -26,12 +26,6 @@ export default function singleSpaAngularJS(userOpts) { | |||
| throw new Error(`single-spa-angularjs must be passed opts.angular`); | |||
| } | |||
|
|
|||
| if (opts.domElementGetter && typeof opts.domElementGetter !== "function") { | |||
There was a problem hiding this comment.
This type check was moved down here 👇 in order to capture the domElementGetter hooks more efficiently.
Because the React version requires all element getters to be functions, where the previous version had a mixture of DOM elements and functions as their references.
| @@ -56,11 +50,12 @@ function mount(opts, mountedInstances, props = {}) { | |||
| return Promise.resolve().then(() => { | |||
| window.angular = opts.angular; | |||
|
|
|||
| const containerEl = getContainerEl(opts, props); | |||
| const domElementGetter = chooseDomElementGetter(opts, props); | |||
There was a problem hiding this comment.
I have changed the naming convention and declaration flow containerEl to match the React version 😄
| } | ||
|
|
||
| function defaultDomElementGetter(props) { | ||
| const appName = props.appName || props.name; |
There was a problem hiding this comment.
I am interested in your thoughts around a breaking change here 😱
The original AngularJS version has the "or" conditions the other way around 🔄 to the React convention.
We could swap them back to stay inline with the AngularJS variant? 🤔
There was a problem hiding this comment.
props.appName existed in single-spa@3, but was removed in single-spa@4 and renamed to name as part of introducing parcels. Single-spa never passes both appName and name props, but it's possible for the user to define either of those in its custom props.
I think the risk of your change is here is very unlikely to break things for single-spa-angularjs users, since it's unlikely people are providing an appName prop as a custom prop. However, I'm happy to publish this PR as a new major version anyway just to strictly follow semver.
| } | ||
|
|
||
| function defaultDomElementGetter(props) { | ||
| const appName = props.appName || props.name; |
There was a problem hiding this comment.
props.appName existed in single-spa@3, but was removed in single-spa@4 and renamed to name as part of introducing parcels. Single-spa never passes both appName and name props, but it's possible for the user to define either of those in its custom props.
I think the risk of your change is here is very unlikely to break things for single-spa-angularjs users, since it's unlikely people are providing an appName prop as a custom prop. However, I'm happy to publish this PR as a new major version anyway just to strictly follow semver.
|
Thanks for taking a look at this and sharing your thoughts @joeldenning 👏 In regards to the AngularJS directive, this can certainly be on our road map (I believe it relates to the ongoing work in this issue #46 ). Currently, we are successfully using SingleSPA's vanilla With this PR (that adds basic AngularJS Parcel support), we have the MVP requirements from a business case perspective to push forward with a SingleSPA implementation 🎉 . As our solution matures, so will our need for abstractions to reduce development friction - and I think an AngularJS Diretive would fit nicely into this direction 🚀 From a timing perspective, this would be a while off and would best suit a minor version bump in the future 🙂 |
|
Sounds good - I'll publish a new version of single-spa-angularjs right now. |
|
I have upgraded to the 4.0 release into our existing Micro Front-end application and can confirm no regressions we encountered 🎉
|
|
Rendering single-spa parcels in AngularJS templates is implemented in #55 |
|
|
What 👋
Address #49 by adding in another
domElementhook that can attach Parcel scenarios inside of parent SingleSPA AngularJS instances.Why 🤔
Parcels are advertised as a core SingleSPA 5 concept, so it only seems natural that the officially supported environments ingest this functionality.
How 💡
To simply get Parcels to work in this repo we only need to add a few lines
I was however asked to emulate the React implementation in terms of their
chooseDomElementGetterflow.This made the final solution more verbose, but I have tried to limit the scope of the changes as much as possible (as there are no tests to protect against refactor regressions).
Where 🔍
The execution involved swapping our this AngularJS function (
getContainerEl) for these React counterparts (chooseDomElementGetter,defaultDomElementGetter, andgetRootDomEl).Note 📋
I will add inline comments around the changes below 👇
Happy to hear feedback/thoughts around bringing over the React implementation. I have been using this branch with our internal SingleSPA application without issues for a few weeks at this point 👍