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: generate different scripts for different projects if they are a part of the same workspace #165

Merged
merged 4 commits into from Apr 11, 2020

Conversation

arturovt
Copy link
Collaborator

No description provided.

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.

Looks good, a couple small comments

src/schematics/ng-add/add-scripts.ts Outdated Show resolved Hide resolved

// Sorts all numbers for ascending order. For example we will get
// `[4200, 4201, 4202]` sorted numbers. We will need `4202` to get next port.
return ports.sort((a, b) => a - b);
Copy link
Member

Choose a reason for hiding this comment

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

The sort function here is unnecessary, right? Each port is already a number due to line 105, so default sort will work?

Suggested change
return ports.sort((a, b) => a - b);
return ports.sort();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Honestly saying haven't used sort in practice, just googled for sort ascending order 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it still has the unnecessary sort function?

Copy link
Member

Choose a reason for hiding this comment

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

Does it not work if you call sort() without the callback? If it works without the callback, let’s remove the callback

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

@arturovt
Copy link
Collaborator Author

@joeldenning ping.

@joeldenning joeldenning merged commit 6ac890d into single-spa:master Apr 11, 2020
@arturovt arturovt deleted the separate-scripts branch April 11, 2020 22:52
@arturovt
Copy link
Collaborator Author

@joeldenning what do you think if we let the user to specify a port when running ng add?
There will be a question right after routing and browserAnimationsModule, the default port will be 4200.

@joeldenning
Copy link
Member

👍 sure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants