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

BREAKING(std/fs): remove readJson and readJsonSync #7255

Merged

Conversation

caspervonb
Copy link
Contributor

@caspervonb caspervonb commented Aug 29, 2020

This removes readJson and readJsonSync from std/fs entirely as they are fairly peculiar functions and the only ones in the std/fs namespace that deals with file input and they only deal with reading JSON from a text file which is very specialized.

The same can be achieved quite easily with JSON.parse(await Deno.readTextFile("path/to/input.json")); as-well as JSON.parse(Deno.readTextFileSync("path/to/input.json")); for the synchronous version.

@caspervonb caspervonb force-pushed the breaking-std-fs-remove-read-json branch from 87b82d6 to 6ba4883 Compare August 29, 2020 17:13
@lcswillems
Copy link

I was happily using them in my code! I think these functions are a nice addition to Deno. JSON is a very common file type especially in JS/TS projects. Moreover, if the same can be achieved easily with a longer code, it is an argument to keep it in Deno because it is easy to main + it makes people's life easier for handling JSON files.

@bartlomieju
Copy link
Member

@lcswillems thanks for the comment. If I'm not mistaken readJson and readJsonSync are legacy functions that were there before we had Deno.readTextFile() API. However I agree with @caspervonb on this one - it's an anti-pattern to add function to std/ which are one-liners or two-liners. It gives a precedent to add more such helpers.

I think we should go ahead with removal in 1.4.0 or at least schedule to deprecate in 1.5.0.

@bartlomieju bartlomieju added this to the 1.4.0 milestone Sep 6, 2020
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @caspervonb

@bartlomieju bartlomieju merged commit d4b6b25 into denoland:master Sep 7, 2020
@exside
Copy link
Contributor

exside commented Sep 14, 2020

Mhm, broke many things here =)...but from an engineering standpoint I agree, was actually about to file an issue that readJson should support JSON.parse() reviver functions... as we have to use JSON.parse anyways, that issue is one less =)...

@caspervonb caspervonb deleted the breaking-std-fs-remove-read-json branch September 15, 2020 00:17
@VienDinhCom
Copy link

VienDinhCom commented Oct 6, 2020

I have ported the readJson, readJsonSync, writeJson & writeJsonSync into this module: https://deno.land/x/jsonfile

caspervonb added a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021
caspervonb added a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb added a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb added a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
caspervonb added a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb added a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb added a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb added a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
caspervonb added a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021
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.

5 participants