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

feat: implement single-spa-angular/parcel library to render parcels #298

Merged
merged 4 commits into from Nov 9, 2020
Merged

Conversation

arturovt
Copy link
Collaborator

@arturovt arturovt commented Nov 6, 2020

No description provided.

@@ -142,9 +151,6 @@
}
},
"lint-staged": {
"*.{ts,html,scss,md,yml}": [
"prettier --write",
"git add"
Copy link
Member

Choose a reason for hiding this comment

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

Does lint-staged automatically stage git files now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't really know :) It was whining that some hook uses git add 🤷‍♂️

if (this.mountParcel === null) {
throw new Error(`
<parcel> was not passed a [mountParcel] binding.
If you are using <parcel> within a module that is not a single-spa application, you will need to import mountRootParcel from single-spa and pass it into <parcel> as a [mountParcel] binding
Copy link
Member

Choose a reason for hiding this comment

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

One thing to note here, is that there is mountRootParcel and mountParcel. The mountParcel function is one that is provided as a prop to all single-spa applications. The two functions are identical, except that mountParcel is bound to the current application, and will automatically unmount the parcel when the application is unmounted.

In React, we automatically use the mountParcel function inside of single-spa-react by using React context to do dependency injection.

Let's update the languaging here to reflect this:

Suggested change
If you are using <parcel> within a module that is not a single-spa application, you will need to import mountRootParcel from single-spa and pass it into <parcel> as a [mountParcel] binding
If you are using <parcel> within a module that is not a single-spa application, you will need to either (1) import mountRootParcel from single-spa or (2) use the `mountParcel` prop provided to single-spa applications. One of those two should be passed into <parcel> as a [mountParcel] binding

if (this.appendTo !== null) {
this.appendTo.appendChild(this.wrapper);
} else {
this.host.nativeElement.appendChild(this.wrapper);
Copy link
Member

Choose a reason for hiding this comment

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

Is host.nativeElement the parent component's wrapper dom element?


const ReactLogo = React.lazy(() => import('./ReactLogo'));

class ReactWidget extends React.Component {
Copy link
Member

Choose a reason for hiding this comment

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

The Angular dev in you is coming out here, in choosing to use a react class component instead of function component 😄 (this is a joke - using class components is completely fine)


// https://docs.github.com/en/free-pro-team@latest/actions/reference/environment-variables#default-environment-variables
const isCI = process.env.CI === 'true';
console.log(`[isCI = ${isCI}]`);
Copy link
Member

Choose a reason for hiding this comment

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

Should this be removed?

@@ -0,0 +1,12 @@
import { BrowserModule } from '@angular/platform-browser';
import { NgModule } from '@angular/core';
import { ParcelModule } from 'single-spa-angular/parcel';
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we have to import ParcelComponent here and add it to the declarations block?

.should('exist')
.get('parcel-root parcel img')
.invoke('attr', 'alt')
.should('eq', 'React logo');
Copy link
Member

Choose a reason for hiding this comment

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

👍 nice!

Copy link
Member

@joeldenning joeldenning left a comment

Choose a reason for hiding this comment

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

Great work

@arturovt arturovt merged commit 23d8f0c into single-spa:master Nov 9, 2020
@arturovt arturovt deleted the parcel branch November 9, 2020 21:04
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.

None yet

2 participants