node-jsonfile icon indicating copy to clipboard operation
node-jsonfile copied to clipboard

use ?? and fs.promises

Open jimmywarting opened this issue 2 years ago • 11 comments

readFile and readFileSync didn't exactly do the same thing. awaiting the readFile should have been inside of the try catch block and respect the throws options. (reading a file might fail)

i also moved some things around for better typing, overriding some let variable with a different type spooks sometimes.


I also wanted to remove graceful-fs and universalify and be completely sync or async (and remove callback method) but thought i would ask first

Where also thinking of switching this from cjs to ESM also if that is alright.

jimmywarting avatar Aug 18 '21 20:08 jimmywarting

readFile and readFileSync didn't exactly do the same thing. awaiting the readFile should have been inside of the try catch block and respect the throws options. (reading a file might fail)

This is a known inconsistency. I'd like to fix it, but I'm not convinced moving the readFile to have an option to completely silence the error is the correct solution. See https://github.com/jprichardson/node-fs-extra/issues/542#issuecomment-360250407 for details.

I also wanted to remove graceful-fs and universalify and be completely sync or async (and remove callback method) but thought i would ask first

In support of removing universalify; let's keep the optional graceful-fs as-is for now.

Where also thinking of switching this from cjs to ESM also if that is alright.

We should support both CJS & ESM; which most likely means keeping the main module CJS, and adding an ESM wrapper. I'd prefer to do this myself, if you don't mind.

RyanZim avatar Sep 23 '21 13:09 RyanZim

Also, please rebase, update GitHub Actions and CircleCI configs to test on the correct versions, and add engines.

RyanZim avatar Sep 23 '21 13:09 RyanZim

@RyanZim but what do you think about TS migration? Are familiar with tsc? By using it we can generate both cjs and esm from one codebase (I hope it'd work for esm).

And why CircleCI should be added if you've already created GitHub workflow?

zardoy avatar Sep 23 '21 19:09 zardoy

@zardoy I don't know TypeScript, so it isn't really an option, since I have to be able to maintain the codebase.

Sorry, GitHub Actions and AppVeyor config, not CircleCI :man_facepalming:

RyanZim avatar Sep 23 '21 19:09 RyanZim

AppVeyor config

I'm just not aware of it. Does it duplicate the GitHub's integration?

zardoy avatar Sep 23 '21 20:09 zardoy

AppVeyor is a working integration, however, it only does Windows, so we're using GitHub Actions for Linux testing (we used to use TravisCI here, but I just migrated over to GH Actions)

RyanZim avatar Sep 23 '21 22:09 RyanZim

I don't think you need typescript just so you can have typing and esm just created this a few days ago: https://jimmywarting.github.io/you-might-not-need-typescript/ 😜

I do however approve the move to do esm-only and a major breaking change 🙃

jimmywarting avatar Nov 21 '21 17:11 jimmywarting

Hmm... this article is insane. However .ts files still have better syntax. Here is more comprehensive response:

nobody likes wasting time compiling stuff.

Everybody knows how slow TSC is (I assume its the slowest compiler in the world). esbuild've had an article which described why so, tsc traverses through the ast of every file like for 5 times: to setup types infering -> linking etc.

However, nobody uses tsc at development, there is esbuild which can compile TypeScript project at any size in a few ms. And the typechecking of full project handling to the CI (or to lint-staged hook).

Just look at Type Guard section, which code looks more concise? I always spend time on setuping esbuid, just to have pretty and convenient syntax for inlining types everywhere. Its easy to do and you can use nice looking and structered code. I agree, that vscode has good suport for writing types in jsdocs though, but most extensions don't understand that.

Some other notes:

  • javascript.suggest.autoImports isn't it already enabled by default?

  • Annotating destructured parameters is wrong, it should be: const { userName, age } = getUser() as DestructuredUser. You can write it without moving your caret up-n-down and even without moving backward with TS postfixes.

Notice how this requires you to write the same properties

  • twice to get the same function signature, so it's more verbose

Yes, it's annoying but it seems nobody knows you should instead:

  1. Write properties as arguments
  2. Use refactor action, which would save a lot of time to you: image Becomes
export function logUser({ userName, age, role, metadata }: { userName: string; age: number; role: 'mod' | 'admin'; metadata: SomethingElse }) {
    // ...
}
  1. Then you should obviously extract it to interface for resuse or leave as is.

The same applies to jsdocs parameters, you shouldn't repeat yourself:

// <- hit ctrl+space here
const transformCode = (code: string, enableSomething: boolean) => {}

You got snippet:

/**
 * 
 * @param code 
 * @param enableSomething 
 */
const transformCode = (code: string, enableSomething: boolean) => {}

vscode will handle futher arg renames (but not adding new)

Generics in JSDoc

I don't agree with you at this point. It is readable enough (if you add some spaces between & of course) since parameters are inlined. And as always with TypeScript you write it faster.

You can even annotate on arrow functions

It would be worth to mention that arrows function shouldn't be used as javascript.suggest.completeFunctionCalls doesn't work with them.

Things TypeScript gets wrong

Didn't get your point here. The same things applies to //@ts-check, otherwise you don't get the typechecking.

My friend also blamed on TypeScript, like he said "why people even use typescript?":

image

Another example with generics:

image

And another example:

image Did I mean string?

(sorry for putting screenshots here)

Error on 1 + true: its like to blame why I can't use promises in if statements (because you shouldn't!). TypeScript just want you to be explicit on + and other places where you expect implicit type converstion. I've heard about flag request that disables checks on binary operators.

Error on alert is also make sense: just don't use alert. We've got amazing console thing in JS.

signal I fixed by using custom lib.d.ts.

However replaceAll is a special one.

One of the stupidest thing of TypeScript is stupid tsconfig.json defaults. I recommend to never use tsconfig (or jsconfig) without shared configuration. Examples: https://github.com/sindresorhus/tsconfig My own is a beast: https://github.com/zardoy/tsconfig

And create some kind of command so you can quickly add it easily to every just created project e.g. something like with mrm.

There are a lot of things, that TypeScript docs will never share with you (such as make tech: another time-saving thing). There are a lot of things that will most probably never be fixed such as inability of TypeScript compiler to remove previous output files. Projects with composite: true would save a lot of time in monorepos by using incremetal build, but it is also requires workaround. I hate tsc and using it only in for building libraries, but even at this point it can do bad things: https://github.com/microsoft/TypeScript/issues/42853

But here what about building libraries:

Libraries that utilizes types in jsdocs can have really good typings support, such as unist ecosystem. Another example is phaser that generated typings from jsdocs, but then migrated to pure .d.ts (as I remember because it was hard to link types).

And finally what about this library. I published typed-jsonfile (it doesn't have readme, but you can look at api at https://paka.dev/npm/typed-jsonfile), that have helpers (for writing package.json files for example):

image

Isn't typescript ecosystem is cool? I hope you now understand what was the intention of my comment above.

Ok, I'm done at this point.

Though, thank you very much for sharing the article! The quality is extremely good. Thank you for using monaco editors.

zardoy avatar Nov 21 '21 19:11 zardoy

@zardoy Long (off topic) post of whatever to use typescript or not, don't mind if you moved it to: https://github.com/jimmywarting/you-might-not-need-typescript/discussions/1 so it can be discuss more over there... if you like... Got a few things i would like to respond back at but it feels too of topic to have that discussion here.

jimmywarting avatar Nov 21 '21 20:11 jimmywarting

Agree, didn't notice how long that post was 😆

zardoy avatar Nov 21 '21 20:11 zardoy

So, finally got moved to https://github.com/jimmywarting/you-might-not-need-typescript/discussions/3

zardoy avatar Nov 21 '21 20:11 zardoy