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

ES2015ify #14

Merged
merged 1 commit into from
Oct 7, 2016
Merged

ES2015ify #14

merged 1 commit into from
Oct 7, 2016

Conversation

schnittstabil
Copy link
Collaborator

@schnittstabil schnittstabil commented Oct 4, 2016

ES2015ify

Closes: #13

Notes:

Further improvements, but BC:

  • Paths in error messages (Compare system errors: ENOENT: no such file or directory, stat 'x'):
- new CpFileError(`cannot read from \`${path}\`: ${err.message}`)
+ new CpFileError(`cannot read from '${path}': ${err.message}`)
// and to be consistent:
- new CpFileError('`src` and `dest` required')
+ new CpFileError(`'src' and 'dest' required`)

Copy link
Owner

@sindresorhus sindresorhus left a comment

Choose a reason for hiding this comment

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

Looks very good!

}
}

CpFileError.prototype.name = 'CpFileError';
Copy link
Owner

Choose a reason for hiding this comment

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

The correct way for ES2015 classes is to set this.name in the constructor instead.

@@ -0,0 +1,35 @@
'use strict';
const EventEmitter = require('events').EventEmitter;
Copy link
Owner

Choose a reason for hiding this comment

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

Can be simplified now:

const EventEmitter = require('events');

this.dest = dest;

let written;
Object.defineProperty(this, 'written', {
Copy link
Owner

Choose a reason for hiding this comment

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

I would keep the private state in a WeakMap and make the written a prototype getter/setter. See: http://www.2ality.com/2016/01/private-data-classes.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, @rauschma rocks 🤘

Copy link

Choose a reason for hiding this comment

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

🙏

@sindresorhus
Copy link
Owner

Paths in error messages (Compare system errors: ENOENT: no such file or directory, stat 'x'):

I don't really care much what Node.js core does. It's not a great example of good error messages. I prefer the backtick as that's what we also use in Markdown to highlight some kind of value.

@schnittstabil schnittstabil force-pushed the es2015ify branch 2 times, most recently from fbff38e to f83ae73 Compare October 4, 2016 19:02
@schnittstabil
Copy link
Collaborator Author

schnittstabil commented Oct 4, 2016

@sindresorhus updated; Thanks for the fast review 🙂

* Remove `readable-stream` (only needed for `v0.10`)
* Remove `mkdirp` `'EEXIST'` checks
* Remove `rewire` in favor of `clear-require` and `require-uncached`

Closes: sindresorhus#13
@sindresorhus sindresorhus merged commit 36349c8 into sindresorhus:master Oct 7, 2016
@sindresorhus
Copy link
Owner

Yay! Very nice work @schnittstabil :)

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