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

Split into multiple files #57

Merged
merged 6 commits into from
Jun 6, 2023
Merged

Split into multiple files #57

merged 6 commits into from
Jun 6, 2023

Conversation

jcbhmr
Copy link
Collaborator

@jcbhmr jcbhmr commented Jun 3, 2023

fixes #69

This PR would...

  • Split each function into its own file
  • Export everything in index.ts just like always

@jcbhmr jcbhmr mentioned this pull request Jun 3, 2023
5 tasks
@jcbhmr jcbhmr requested a review from mesqueeb June 3, 2023 04:25
@jcbhmr jcbhmr self-assigned this Jun 3, 2023
@mesqueeb
Copy link
Owner

mesqueeb commented Jun 3, 2023

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
Copy link
Collaborator Author

jcbhmr commented Jun 3, 2023

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 airbnb/javascript#1365 where Airbnb explains why they use default exports

@jcbhmr jcbhmr marked this pull request as draft June 3, 2023 04:44
@mesqueeb
Copy link
Owner

mesqueeb commented Jun 3, 2023

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
Copy link
Owner

mesqueeb commented Jun 3, 2023

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.

This can be further discussed in #55

More discussion on the pros and cons of default exports vs named exports has been discussed to death in airbnb/javascript#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
Copy link
Collaborator Author

jcbhmr commented Jun 3, 2023

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! 😊👍

@jcbhmr jcbhmr reopened this Jun 3, 2023
@jcbhmr jcbhmr removed the request for review from mesqueeb June 3, 2023 21:48
@jcbhmr jcbhmr marked this pull request as ready for review June 3, 2023 22:14
@jcbhmr jcbhmr requested a review from mesqueeb June 3, 2023 22:14
@jcbhmr jcbhmr linked an issue Jun 6, 2023 that may be closed by this pull request
@mesqueeb mesqueeb merged commit 2ab780a into production Jun 6, 2023
@mesqueeb mesqueeb deleted the multiple-files branch June 6, 2023 20:18
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.

Split src/index.ts into multiple files
2 participants