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.

Sign up for GitHub

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 Parcel Support [#49] #50

Merged
merged 1 commit into from
Sep 26, 2020
Merged

Feature: Add Parcel Support [#49] #50

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 52 additions & 20 deletions src/single-spa-angularjs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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") {
Copy link
Contributor Author

@devonChurch devonChurch Sep 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

throw new Error(
`single-spa-angularjs opts.domElementGetter must be a function`
);
}

if (!opts.mainAngularModule) {
throw new Error(
`single-spa-angularjs must be passed opts.mainAngularModule string`
Expand All @@ -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);
Copy link
Contributor Author

@devonChurch devonChurch Sep 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I have changed the naming convention and declaration flow containerEl to match the React version 😄

const domElement = getRootDomEl(domElementGetter, props);
const bootstrapEl = document.createElement("div");
bootstrapEl.id = opts.elementId;

containerEl.appendChild(bootstrapEl);
domElement.appendChild(bootstrapEl);

if (opts.uiRouter) {
const uiViewEl = document.createElement("div");
Expand Down Expand Up @@ -94,7 +89,10 @@ function mount(opts, mountedInstances, props = {}) {
function unmount(opts, mountedInstances, props = {}) {
return new Promise((resolve, reject) => {
mountedInstances.instance.get("$rootScope").$destroy();
getContainerEl(opts, props).innerHTML = "";
const domElementGetter = chooseDomElementGetter(opts, props);
const domElement = getRootDomEl(domElementGetter, props);

domElement.innerHTML = "";

if (opts.angular === window.angular && !opts.preserveGlobal)
delete window.angular;
Expand All @@ -103,22 +101,56 @@ function unmount(opts, mountedInstances, props = {}) {
});
}

function getContainerEl(opts, props) {
let element;
if (opts.domElementGetter) {
element = opts.domElementGetter(props);
function chooseDomElementGetter(opts, props) {
if (props.domElement) {
return () => props.domElement;
} else if (props.domElementGetter) {
return props.domElementGetter;
} else if (opts.domElementGetter) {
return opts.domElementGetter;
} else {
const htmlId = `single-spa-application:${props.name || props.appName}`;
element = document.getElementById(htmlId);
if (!element) {
element = document.createElement("div");
element.id = htmlId;
document.body.appendChild(element);
return defaultDomElementGetter(props);
}
}

function defaultDomElementGetter(props) {
const appName = props.appName || props.name;
Copy link
Contributor Author

@devonChurch devonChurch Sep 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

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.

if (!appName) {
throw Error(
`single-spa-angularjs was not given an application name as a prop, so it can't make a unique dom element container for the angularjs application`
);
}
const htmlId = `single-spa-application:${appName}`;

return function defaultDomEl() {
let domElement = document.getElementById(htmlId);
if (!domElement) {
domElement = document.createElement("div");
domElement.id = htmlId;
document.body.appendChild(domElement);
}

return domElement;
};
}

function getRootDomEl(domElementGetter, props) {
if (typeof domElementGetter !== "function") {
throw new Error(
`single-spa-angularjs: the domElementGetter for angularjs application '${
props.appName || props.name
}' is not a function`
);
}

const element = domElementGetter(props);

if (!element) {
throw new Error(`domElementGetter did not return a valid dom element`);
throw new Error(
`single-spa-angularjs: domElementGetter function for application '${
props.appName || props.name
}' did not return a valid dom element. Please pass a valid domElement or domElementGetter via opts or props`
);
}

return element;
Expand Down