Skip to content

Use yarn #241

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

Closed
wants to merge 1 commit into from
Closed

Use yarn #241

wants to merge 1 commit into from

Conversation

kmannislands
Copy link

Tiny check in for a tiny improvement in package management.

  • add dev dependency on yarn
  • check in yarn lock file for better dependency version consistency across devs/environments

TLDR; New packages should be installed with yarn add instead of npm install

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.51% when pulling 33efdff on kmannislands:i238-use-yarn into 0249c91 on share:master.

@alecgibson
Copy link
Collaborator

Why use yarn instead of npm? Can't we just check in npm's package-lock.json? (Something I've been meaning to do for a while)

@kmannislands
Copy link
Author

@alecgibson I find yarn convenient because:

  • works across node version pre-npm 5 support
  • puts a cdn in front of npm registry that works better for devs in china/can speed things up
  • haven't seen nearly the high profile bug reports out of yarn as npm
  • just personally prefer yarn's syntax slightly

Mainly I just made the check so I wouldn't have to git clean -df all the time when I use yarn to install if I start making contributions to ShareDB.
I would be willing to make a PR to check in package-lock.json instead and to .gitignore the yarn.lock instead

@alecgibson
Copy link
Collaborator

Given that npm is the more "core" service (ie the one we publish to), I'd personally prefer to stick to just that (in the same way that there's a strong leaning towards using standard JavaScript rather than eg transpiling - it makes working on the repository a lot more universal and friendly to the uninitiated).

@kmannislands
Copy link
Author

kmannislands commented Aug 29, 2018

Amounts to the preferences of the team. I'll close this PR and open one to add yarn's lockfile to .gitignore
As for the 'standard js' being more accessible, I find that fairly debatable. Perusing the code, I found quite a bit of stuff that I think would be significantly more accessible if it was written with es5. Ie from the codebase:

function MemoryDB(options) {
  if (!(this instanceof MemoryDB)) return new MemoryDB(options);
  DB.call(this, options);

  // Map from collection name -> doc id -> doc snapshot ({v:, type:, data:})
  this.docs = {};

  // Map from collection name -> doc id -> list of operations. Operations
  // don't store their version - instead their version is simply the index in
  // the list.
  this.ops = {};

  this.closed = false;
};
module.exports = MemoryDB;

MemoryDB.prototype = Object.create(DB.prototype);

MemoryDB.prototype.close = function(callback) {
  this.closed = true;
  if (callback) callback();
};

Is harder to parse than the es5 class equivalent:

export class MemoryDB {
  // Map from collection name -> doc id -> doc snapshot ({v:, type:, data:})
  docs = {};
  
  // Map from collection name -> doc id -> list of operations. Operations
  // don't store their version - instead their version is simply the index in
  // the list.
  ops = {};

  closed = false;

  close(callback) {
    this.closed = true;
    if (callback) callback();
  }

  constructor(options) {
    DB(options);
  }
}

@alecgibson
Copy link
Collaborator

ES5 doesn't support classes. Do you mean ES6? We don't use that, because of the joys of IE. Using ES6 would require either backend-only code (like the new sharedb-milestone-mongo repo I'm trying to get started), or a move to transpiling, which could be argued for but you'd need to argue for it. You can pile opinions in to existing discussion here: #202

@kmannislands
Copy link
Author

@alecgibson yes, good catch, I meant ES6. Plus I used class-properties in the example, so I guess I'm just talking some language that doesn't truly exist outside of babel or proposals 😄
Cool, I was considering opening such a ticket myself, thanks for pointing me.

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.

3 participants