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

switched to yarn #66

Merged
merged 8 commits into from Mar 15, 2017
Merged

switched to yarn #66

merged 8 commits into from Mar 15, 2017

Conversation

@artlowel
Copy link
Member

artlowel commented Feb 17, 2017

This PR connects to #65

It replaces npm with yarn to manage the dependencies of the project

We shouldn't merge this until at least one person using linux and one person using windows has verified that it works.

Note: install using npm

I opted to add yarn to the global install script and install it using npm, that means people don't need to install yarn manually in advance, and our quick start guide can remain short, but it isn't recommended by the yarn devs - they advise you to use the installer. On my system I installed both, and the installer version gets priority over the node version, but that's likely up to the order they're added to $PATH and is by no means a guarantee.

What do you think? Should we install it automatically, or have people install it manually?

README.md Outdated
@@ -53,14 +53,14 @@ Not sure where to start? watch the training videos linked in the [Introduction t
You can find more information on the technologies used in this project (Angular 2, Typescript, Angular Universal, RxJS, etc) on the [DuraSpace wiki](https://wiki.duraspace.org/display/DSPACE/DSpace+7+UI+Technology+Stack)

## Requirements
* [Node.js](https://nodejs.org) and [npm](https://www.npmjs.com/)
* Ensure you're running node >= `v5.x` and npm >= `v3.x`
* [Node.js](https://nodejs.org) and [yarn](https://yarnpkg.com)

This comment has been minimized.

Copy link
@tdonohue

tdonohue Feb 23, 2017

Member

NPM is likely still required, correct?

This comment has been minimized.

Copy link
@artlowel

artlowel Feb 24, 2017

Author Member

Yes that shouldn't have been removed. I'll fix it

Reinstated npm as a dependency
@hardyoyo

This comment has been minimized.

Copy link
Member

hardyoyo commented Mar 1, 2017

I can verify this works as expected on Linux, however, one warning seemed a bit concerning:

> spawn-sync@1.0.15 postinstall /usr/local/lib/node_modules/yarn/node_modules/spawn-sync
> node postinstall

- mimic-fn@1.1.0 node_modules/yarn/node_modules/mimic-fn
/usr/local/lib
├─┬ angular-cli@1.0.0-beta.28.3 
│ ├─┬ cssnano@3.10.0
│ │ └─┬ postcss-merge-rules@2.1.2
│ │   └── postcss-selector-parser@2.2.3 
│ ├── rimraf@2.6.1 
│ └── UNMET PEER DEPENDENCY rxjs@^5.0.1
... /* time passes */ ...
npm WARN @angular/core@2.4.8 requires a peer of rxjs@^5.0.1 but none was installed.

observed while running yarn run global

@artlowel

This comment has been minimized.

Copy link
Member Author

artlowel commented Mar 2, 2017

@hardyoyo That issue isn't specific to yarn, it's that the angular-cli package we had in the global script has since been renamed to @angular/cli and of course the old angular-cli package is no longer updated, so is no longer compatible with the dependencies in our project which have been updated since
I changed it to the new name that fixes the problem for me.

@atarix83

This comment has been minimized.

Copy link
Contributor

atarix83 commented Mar 9, 2017

Hi,
I finally tried yarn on windows and it all works properly.
I agree with global installation of yarn, it's the quickest solution.

@artlowel

This comment has been minimized.

Copy link
Member Author

artlowel commented Mar 10, 2017

I agree with global installation of yarn, it's the quickest solution.

I've changed my mind on this. Yes it is slightly quicker to do the initial install, but in the full install docs we do recommend to install yarn properly. And if you do both, you can end up with an uncertainty about which version is being used.
I have therefore added yarn as a prerequisite to the quick install as well, and removed it from the global script.

@artlowel artlowel merged commit 6402456 into DSpace:master Mar 15, 2017
@artlowel artlowel removed the needs review label Mar 15, 2017
@artlowel artlowel deleted the artlowel:yarn branch Mar 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.