Closed vladfrangu closed 3 years ago
overall, I think keeping everything in
fs
andpath
(without destructuring) helps keeping track what's going on. the amount of "flying" functions without the namespace can be overwhelming 😄
Can do! I'm personally more used to destructuring, but I can revert that!
also, I see no evil using
export default
in place ofmodule.exports
, might be easier to disambiguate when importing just the types, since most of the time 1 file contains 1 class only
export default
is annoying, especially for any JS users (Case in point with ow's const ow = require('ow').default
), while export
directly (as it is right now) makes the import be written the same both in JS and TS (which personally I find more appropriate and easy to do). Let me know your thoughts tho! :D
I am on Vlad's side, I do prefer destructing when it comes to ES imports - actually it is not even destructing, as fs
is a module with functions, it is not an object with methods. Btw even nodejs docs are using this approach too in their examples. Regarding default exports, they are evil and should be generally avoided (sure there are exceptions, as with everything).
death to commonjs 🙈 anyways, for two lines of code, that makes sense. not so much for full codebases
So can I merge this or are changes still needed guys? @B4nan @pocesar ?
Would be probably good to test this bit first:
But its mergable for sure, as it keeps the old behaviour.
Btw, I would also prefer fs.remove
instead of remove
and so on. Without the fs
it's an extremely generic name and one always needs to go back to the definition to see what it actually means and does.
Yes, but going to definition is a matter of using a shortcut (cmd + y
in webstorm, I'd expect sth similar in vscode too), or even just hovering over can be enough. I usually tend to use destructing imports as this is how the IDE does it for me automatically.
I believe the main goal behind the movement toward this was to support threeshaking, which is not really valid for us here.
Not a strong opition on this, just saying that I would do it the same way, so I am ok with it.
@vladfrangu I'll get some opinions from the company whether we want to standardize this or not and get back to you :)
@vladfrangu Apparently there general agreement is that keeping the containing namespace makes sense if it holds informational value like.
import fs from 'fs-extra';
import _ from 'underscore';
// but
import { foo, bar } from 'utils';
But noone really has a strong opinion about this. So let's do whatever you find best.
@mnmkng gotcha! Personally, I'd stick to leaving it how it is, mainly because that's how editors import them automatically too (and as an example, if I write writeFile
then CTRL + Space
, vsc will popup all options with that name; I select the fs-extra
one and it'll import it like that 👀. Pretty sure the same is applicable to WebStorm/IntelliJ, however I don't use either [for TypeScript/JavaScript/Web development]), instead of having to manually import the whole namespace before being able to use the functions you need. Nonetheless, I'll gladly do whatever suits you guys best!
Other than that, for this PR I just need to verify the stream TODO I left, which @pocesar left a message on, otherwise it should LGTM!
CC: @pocesar @B4nan