Skip to content

Conversation

@kangyizhang
Copy link
Contributor

@kangyizhang kangyizhang commented Oct 7, 2019

Currently scripts/install.js add a field in package.json during installation and then remove it at the end of installation. But when saving the package.json file it removes the last line and cause unexpected change.

This PR add a newline at the end of package.json when saving it, and use writeFileSync.


This change is Reviewable

@kangyizhang kangyizhang requested a review from caisq October 7, 2019 21:52
@kangyizhang kangyizhang changed the title Add new line when saving package.json through code [tfjs-node] Add new line when saving package.json through code Oct 8, 2019
Copy link
Contributor

@caisq caisq left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @caisq and @kangyizhang)


tfjs-node/scripts/install.js, line 69 at r1 (raw file):

writeFileSync(

Why is it necessary to change writeFile() towriteFileSync() in this PR? If you want to change this, you should change the error callback function (the 3rd argument) as well, right?

Copy link
Contributor Author

@kangyizhang kangyizhang left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @caisq)


tfjs-node/scripts/install.js, line 69 at r1 (raw file):

Previously, caisq (Shanqing Cai) wrote…
writeFileSync(

Why is it necessary to change writeFile() towriteFileSync() in this PR? If you want to change this, you should change the error callback function (the 3rd argument) as well, right?

It's not necessary in this PR and you are right the callback argument should be removed. Now if writeFileSync failed it will throw an exception.

Using writeFileSync will make sure the new field is added in package.json before node-gyp binding read the package.json file.

@kangyizhang kangyizhang merged commit f06434a into master Oct 8, 2019
@kangyizhang kangyizhang deleted the node-nit branch October 9, 2019 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants