Infer EOLs from existing file or use OS EOL #1405
Conversation
|
@bestander Will try, thanks. |
Implemented as proposed by @ahelmberger Resolves #1061
|
@bestander Added, please check. |
| const cr = new Buffer('\r', 'utf8')[0]; | ||
| const lf = new Buffer('\n', 'utf8')[0]; | ||
|
|
||
| async function getEolFromFile(path: string) : Promise<string | void> { |
There was a problem hiding this comment.
The unfortunate bit is that it adds a read file to every write file operation.
That could unexpectedly slow down quite a few things.
Any ideas how to apply this only to yarn.lock/package.json writing?
There was a problem hiding this comment.
@bestander
IMHO putting the line ending setting into .yarnrc would be not so nice when team members work on different OSs, which is quite probable for a relatively big project.
I tried to measure the overhead and got about 20ms for 10000 read iterations, so for one read it should not be significant.
However, we could limit this behaviour to only files included into source control. Is it only yarn.lock and package.json? If so, maybe we could expose both write functions and use them selectively?
There was a problem hiding this comment.
I would use this explicitly for yarn.lock, package.json and .yarnrc files only then.
How about fs would expose fs.writeFilePreservingEol and we would use it sparsely?
This kind of file parsing does not sit well with me.
There was a problem hiding this comment.
Makes sense. Updated.
Implemented as proposed by @ahelmberger Resolves #1061
* Set UID and GID of untared files to zero if user is root * Remove second platform check
This PR resolves issue #1857.
* Add very basic test for global add * Add assertion & Fix typo
I don't think we need this any more
* Skip setting hash if hash was not set previously * Add end to end test for installing a github package * Update README.md * Fix eslint * Reverse accidental fixture changes * Add unit tests * No end to end tests for github installs
| async function getEolFromFile(path: string) : Promise<string | void> { | ||
| let buffer; | ||
| try { | ||
| buffer = await readFileBuffer(path); |
There was a problem hiding this comment.
better return undefined if files does not exist (check if it exists first).
And don't catch error in readFile
There was a problem hiding this comment.
I thought about it, but if the file is inaccessible for some other reason - is it OK to just let it fail here? Will this be handled on a higher level?
… built-ins (#1790) The reason for this is that Yarn is useful in many JS environments such as browsers and React Native, which don't provide the Node built-ins. Depending on polyfills is the right thing to do in that scenario and the reality is that people intentionally publish these polyfills under the same name as the Node built-ins (e.g. url and buffer).
Implemented as proposed by @ahelmberger Resolves #1061
|
Commits still look quite broken in the PR. |
|
@bestander Sorry I messed up with rebasing :( Created a new PR: #1911 |
Summary
Yarn uses UNIX line endings on all systems when writing lock file. We want to solve this problem as described in #1061: infer line endings from the existing file, and if it does not exist, use the system default.
Test plan
On a UNIX system, in an empty directory:
$ yarn add <some_package>
$ cat -e yarn.lock
You should only see LF line endings.
$ unix2dos yarn.lock
$ yarn add <some_other_package>
$ cat -e yarn.lock
You should only see CRLF line endings.
On a Windows system, in an empty directory:
$ yarn add <some_package>
$ cat.exe -e yarn.lock
You should only see CRLF line endings.
$ dos2unix.exe yarn.lock
$ yarn add <some_other_package>
$ cat -e yarn.lock
You should only see LF line endings.