mesqueeb / is-what

JS type check (TypeScript supported) functions like `isPlainObject() isArray()` etc. A simple & small integration.
https://mesqueeb.github.io/is-what/
MIT License
170 stars 18 forks source link

Split into multiple files #57

Closed jcbhmr closed 1 year ago

jcbhmr commented 1 year ago

fixes #69

This PR would...

mesqueeb commented 1 year ago

Thank you so much! I'd love to merge this.

I'm not really willing to change the source code as much as this PR though. I don't have any issues with separating everything in files, but I rather like how this source code works and looks currently:

export function isNegativeNumber(payload: any): payload is number {
  return isNumber(payload) && payload < 0
}
export function isBoolean(payload: any): payload is boolean {
  return getType(payload) === 'Boolean'
}
export function isRegExp(payload: any): payload is RegExp {
  return getType(payload) === 'RegExp'
}
export function isMap(payload: any): payload is Map<any, any> {
  return getType(payload) === 'Map'
}
export function isWeakMap(payload: any): payload is WeakMap<any, any> {
  return getType(payload) === 'WeakMap'
}
// etc.

And this PR is kind of a big change... It would feel to me like the source code is not mine anymore and in many places I have harder time reading it. So I'd like to request to retain the source code as-is for this PR, you can move getType to a utils.ts file or something and import from there, and then any changes to the source code on a function-by-function basis can be discussed separately in separate PRs. I don't think I want to change too much to the source code unless there's an objective benefit over my initial code.

Another thing I'd love to request to change is to not use default exports. I do not use default exports in any of my code anywhere as kind of a personal rule of thumb. There were in the past many cjs/esm issues with default exports that all went away not using it. So I'd like to request to only use named exports for this PR.

PS: Not sure if you saw my email, but I'm available to chat on Discord if you want. My username is: Luca Ban [Mesqueeb]#4774

jcbhmr commented 1 year ago

I'm not really willing to change the source code as much as this PR though. I don't have any issues with separating everything in files, but I rather like how this source code works and looks currently:

export function isNegativeNumber(payload: any): payload is number {
  return isNumber(payload) && payload < 0
}
export function isBoolean(payload: any): payload is boolean {
  return getType(payload) === 'Boolean'
}
export function isRegExp(payload: any): payload is RegExp {
  return getType(payload) === 'RegExp'
}
export function isMap(payload: any): payload is Map<any, any> {
  return getType(payload) === 'Map'
}
export function isWeakMap(payload: any): payload is WeakMap<any, any> {
  return getType(payload) === 'WeakMap'
}
// etc.

Problem with this is that it discourages documentation like examples. For instance, that isBoolean(). Does it work ONLY on new Boolean() objects? Or on primitives too? Does it work on custom class Boolean {} objects? What about { [Symbol.toStringTag]: "Boolean" } objects? None of that is documented 😭. That's kinda why I wanted individual files: to push for more documentation since you are no longer incentivized to compress things.

And this PR is kind of a big change... It would feel to me like the source code is not mine anymore and in many places I have harder time reading it. So I'd like to request to retain the source code as-is for this PR, you can move getType to a utils.ts file or something and import from there, and then any changes to the source code on a function-by-function basis can be discussed separately in separate PRs. I don't think I want to change too much to the source code unless there's an objective benefit over my initial code.

👍

Another thing I'd love to request to change is to not use default exports. I do not use default exports in any of my code anywhere as kind of a personal rule of thumb. There were in the past many cjs/esm issues with default exports that all went away not using it. So I'd like to request to only use named exports for this PR.

I think default exports are great! They make is so easy to import one thing from a package and really natively signify that something is the main export of a file. This works great when eachTing.ts is its own file. For collection-of-things.ts files, yes I agree that named exports are better. I am of the opinion that one-thing-per-file pairs great with default exports. But I will conform to whatever things you want; this is your project after all. 😊

On the CJS note: I think that CJS is in decline. This ties into my question about whether or not you want to #47 which would solve the #37.

More discussion on the pros and cons of default exports vs named exports has been discussed to death in https://github.com/airbnb/javascript/issues/1365 where Airbnb explains why they use default exports

mesqueeb commented 1 year ago

Problem with this is that it discourages documentation like examples. For instance, that isBoolean(). Does it work ONLY on new Boolean() objects? Or on primitives too? Does it work on custom class Boolean {} objects? What about { [Symbol.toStringTag]: "Boolean" } objects? None of that is documented 😭. That's kinda why I wanted individual files: to push for more documentation since you are no longer incentivized to compress things.

@jcbhmr I think you misunderstood something. I mentioned I have no issues splitting it up into multiple files and agree that adding more JSDocs to each file is a great idea. I meant that I want to keep the source-code of the function bodies the same for now. If the PR can just split up the files, but keep the source-code as is, it'll be way easier to merge that first, then continue discussion source-code changes in individual PRs per function.

What I mean is, no need to change

// this:
export function isBoolean(payload: any): payload is boolean {
  return getType(payload) === 'Boolean'
}
// to this:
export default function isBoolean(x: unknown): x is boolean {
   return typeof x === "boolean";
 }

or

// this
export function isRegExp(payload: any): payload is RegExp {
  return getType(payload) === 'RegExp'
}
// to this
export default function isRegExp(x: unknown): x is RegExp {
   try {
     Object.getOwnPropertyDescriptor(RegExp.prototype, "source")!.get!.call(
       x as RegExp
     );
   } catch {
     return false;
   }
   return true;
 }

So if eg. you make a file called isBoolean.ts I would want to see something like this:

import { getType } from './utils.ts'
/** any JSDoc... feel free to add improvements here */
export function isBoolean(payload: any): payload is boolean {
  return getType(payload) === 'Boolean'
}

This is what I meant.

Otherwise if you do everything at once, there's no easy way for me to discuss changes and clearly see what changed where. 😅

mesqueeb commented 1 year ago

On the CJS note: I think that CJS is in decline. This ties into my question about whether or not you want to https://github.com/mesqueeb/is-what/issues/47 which would solve the https://github.com/mesqueeb/is-what/issues/37.

This can be further discussed in https://github.com/mesqueeb/is-what/pull/55

More discussion on the pros and cons of default exports vs named exports has been discussed to death in https://github.com/airbnb/javascript/issues/1365 where Airbnb explains why they use default exports

it's a bit much to parse through. I am not seeing where exactly AirBnB's reasoning is in this thread. But a quick glance and this caught my eye:

  • Named exports make it clear throughout the application that you are using a specific value. Using default exports means that each part of the app may have a different name for the same function which makes code harder to reason about.
  • The argument about changing the name requiring all other parts of the application needing to do the same WAS VALID at one point, but with features like Rename Symbol which most IDE's - and VSCode (which lets be real most of us are using) have -- this became 100% useless.
  • Named exports make things like Auto Imports that some language features such as TypeScript support. Without named exports, they are far more prone to issue and require context before they are ever capable of suggesting an auto import of a given value.
  • If you end up needing to export more values from a file as the application grows, it ends up becoming quite messy to either change from default to named as this rule seems to want -- or move to more files being used. With the more files approach - if you want to keep things logically organized that may even mean you have to move things into a folder and make significant changes to the design of the project as it grows and can end up highly disorganized Whereas if you default to named exports then nothing changes, you simply export more values. If you want to use folder approach, you can - just do the folder, create an index file and export named exports from that - and dependent files have no knowledge of the change required.

This is kinda where I'm at. This and also all of the issues I've had in the past. Literally 10+ hours wasted in tooling hell and it all went away by using named exports.

jcbhmr commented 1 year ago

I think you misunderstood something. I mentioned I have no issues splitting it up into multiple files and agree that adding more JSDocs to each file is a great idea. I meant that I want to keep the source-code of the function bodies the same for now. If the PR can just split up the files, but keep the source-code as is, it'll be way easier to merge that first, then continue discussion source-code changes in individual PRs per function.

Yep! I misunderstood. Thanks for clearing that up for me! 😊👍