Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Sort create-project dependencies and devDependencies #867

Merged
merged 2 commits into from
May 29, 2018

Conversation

helfi92
Copy link
Member

@helfi92 helfi92 commented May 11, 2018

Resolves #853.

@helfi92 helfi92 self-assigned this May 11, 2018
@helfi92 helfi92 requested a review from edmorley May 11, 2018 02:43
@edmorley edmorley removed their request for review May 14, 2018 10:06
@edmorley
Copy link
Member

create-project may be removed as part of #852 so cancelling the review for now

@eliperelman eliperelman requested a review from a team May 23, 2018 21:28
@eliperelman
Copy link
Member

Looks like we need a rebase here.

@helfi92 helfi92 force-pushed the sort-dependencies branch from 6cc2348 to 07c6808 Compare May 24, 2018 21:40
@helfi92
Copy link
Member Author

helfi92 commented May 24, 2018

Rebased.

Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

The sort order with this gives eg:
eslint, jest, neutrino, @neutrinojs/airbnb, @neutrinojs/jest, @neutrinojs/react, webpack, webpack-cli, webpack-dev-server

Looks neater, though I'm on the fence as to whether the added complexity of a custom sort function is worth it, since the default .sort() would give:
@neutrinojs/airbnb, @neutrinojs/jest, @neutrinojs/react, eslint, jest, neutrino, webpack, webpack-cli, webpack-dev-server

...which is consistent with the package.json order, even if the neutrino deps aren't all consecutive.

Is up to you :-)

@helfi92
Copy link
Member Author

helfi92 commented May 26, 2018

No strong feelings here. Feel free to close it if it's too much added complexity :)

@edmorley
Copy link
Member

I don't mean close, I mean dependencies.sort() vs dependencies.sort(customSortFunction) :-)

Resort to Array.prototype.sort instead.
@helfi92 helfi92 requested a review from edmorley May 26, 2018 19:00
Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Many thanks!

(Travis is failing - I believe due to a bug with either verdaccio or lerna when the auto-generated test package version SHA is numeric only and starting with a zero. I'll, try and track this down separately, but needn't block merging)

@edmorley edmorley merged commit 160e12d into neutrinojs:master May 29, 2018
@edmorley edmorley added this to the v9 milestone Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants