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

fix(betterer 🐛): improve results file escaping #372

Merged
merged 1 commit into from
Nov 3, 2020

Conversation

phenomnomnominal
Copy link
Owner

Fixes #371

@phenomnomnominal phenomnomnominal merged commit 0ed1c70 into master Nov 3, 2020
@phenomnomnominal phenomnomnominal deleted the escape-interpolation-tokens branch November 3, 2020 01:02
@Svish
Copy link

Svish commented Nov 3, 2020

Out of curiosity, why do you do this module require and escaping stuff? Wouldn't it be easier to just use the regular JSON parse and stringify? Or is there a reason why you do what you do instead?

(Not done much Node development, only browser stuff, so just curious🙂)

@phenomnomnominal
Copy link
Owner Author

There's a few reasons I went with it, although I'm sure I could convince myself out of it if I did it again!

  1. JSON files can't have comments. Since this file is kind of special, it'd be nice to have a header that explained why the file exists and that it shouldn't be modified. If I wanted JSON + the header, I'd have to do custom file handling anyways, so leaning on JS (and require) makes sense.

  2. Custom formatting. Having a custom file format means I can be a little bit more particular about how the file is formatted, and therefore write automated updates if I need to change the format. This would be possible with JSON, and of course people could still change the .results file, but people get a bit more nervous about modifying custom file formats (like yarn.lock files or Jest snapshots)

  3. The .results file basically is a Jest snapshot - the file format was inspired by Jest, and this PR actually made me think I should probably try to lean more on the Jest internals!

@Svish
Copy link

Svish commented Nov 3, 2020

Cool, thanks, makes sense 🙂👍

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.

BUG: "Could not read results" on large result file?
2 participants