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

ES6 classes and Advanced Typings #13

Open
wants to merge 63 commits into
base: master
Choose a base branch
from

Conversation

pabadm
Copy link

@pabadm pabadm commented Sep 21, 2023

Hello. I decided to remake implementation of your callable-instance. (Switched to ES6 class syntax and ES6 Symbols).

  1. Now Callable also accepts its Symbol for CALL method. (old propertyName in constructor also works)
  2. Does not copy length because CALL method can be overwritten. (has length: 0 like all ((...args) => any) functions
  3. Does not forward function name. Instead forwards constructor.name
  4. Made advanced typings to easily forward property and extend CreatedClass
  5. Built index.js using ts-compiler

If you don`t mind Accepting my pull request, let me know first so I can create Readme.

@CGamesPlay
Copy link
Owner

Thanks for your interest in contributing! Wow, this completely rewrites the package. I'm not opposed to accepting, but there is a laundry list of changes I'd like to see before doing so 🙂

Overall questions/comments:

  1. Where does your interest in this package come from? Are you using this package in production and find it limiting?
  2. Let's please keep the dist directory in .gitignore.
  3. What is OverrideCall?
  4. Can you update .github/workflows/node.js.yml to test [16.x, 18.x, 20.x, 22.x, 24.x]? That should fix the CI as well.
  5. Please run Prettier on all the files (default config).

Then there are some inline comments.

@pabadm
Copy link
Author

pabadm commented Sep 28, 2023

Interest came from idea of callable objects in JS. I decided to change implementation because of setPrototypeOf https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/setPrototypeOf.
My variant is inspired by this article (the bind way) https://medium.com/@adrien.za/creating-callable-objects-in-javascript-fbf88db9904c.
I decided to make something like OverrideCall because of typescript limitations. With OverrideCall its easy to change type of Call signature if you extend Callable class. For Example MyCallable extends Callable. MyNewClass extends MyCallable. So you can write MyNewClass extends (MyCallable as OverrideCall)
Also i thought of keeping this package for legacy pre ES5 implementation with updated types and posting other package es6-callable with ES6 syntax.

@CGamesPlay
Copy link
Owner

Everything in dist is still in the repo. You ignored only future files. You'll need to remove the files and commit the removals as well.

I decided to make something like OverrideCall because of typescript limitations. With OverrideCall its easy to change type of Call signature if you extend Callable class. For Example MyCallable extends Callable. MyNewClass extends MyCallable. So you can write MyNewClass extends (MyCallable as OverrideCall)

Got it. An example would be very useful in the README.

Also i thought of keeping this package for legacy pre ES5 implementation with updated types and posting other package es6-callable with ES6 syntax.

This change will be a new major version, so we can have a note that says that ES5-compatible implementation lives in ~2.0.0.

Checks look good, please run Prettier on the source files and update the README as you wanted to. I'll proofread the README and then we can publish this as version 3.0.0.

Copy link
Owner

@CGamesPlay CGamesPlay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apparently adding my top-level comment left all these in pending, apologies.

src/index.js Outdated Show resolved Hide resolved
src/test/typeCheck.ts Outdated Show resolved Hide resolved
src/test/typeCheck.ts Outdated Show resolved Hide resolved
src/test/typeCheck.ts Outdated Show resolved Hide resolved
src/test/typeCheck.ts Outdated Show resolved Hide resolved
src/test/test.cjs Outdated Show resolved Hide resolved
src/test/test.cjs Outdated Show resolved Hide resolved
src/test/test.cjs Outdated Show resolved Hide resolved
src/test/test.cjs Outdated Show resolved Hide resolved
src/test/test.mjs Outdated Show resolved Hide resolved
@pabadm
Copy link
Author

pabadm commented Oct 4, 2023

apparently adding my top-level comment left all these in pending, apologies.

Will be finished. just dont have time now. I will comment when module is ready

Copy link
Owner

@CGamesPlay CGamesPlay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, apologies for the delay on this. I reviewed the library code and it all looks great, with just a few minor inlines.

I will review the tests as well ASAP. Don't worry about making the inline changes; I can do that when I review the tests. If the tests look good, I will go ahead and merge into main and release the version to NPM.

How would you like to be credited in the README?

.gitignore Outdated
@@ -36,4 +36,5 @@ jspm_packages
# Optional REPL history
.node_repl_history

/dist
# dist
dist
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this was a meaningful change, either remove it or remove the useless comment.

lib/index.d.ts Outdated
@@ -0,0 +1,78 @@
declare module "callable-instance" {
const CALL: unique symbol;
export type SCALL = typeof CALL;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should remove this type alias. It's not saving much and it adds an extra concept for people to be confused by. In this file we can use typeof CALL directly, and any consumer can use typeof Callable.CALL.

"types": "index.d.ts",
"main": "./lib/index.js",
"types": "./lib/index.d.ts",
"type": "commonjs",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also add sideEffects: false to this.

// expectType<Function>(new RepeaterWithTSClassOverride(5));
// expectType<Object>(new RepeaterWithTSClassOverride(5));
// });
// });
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove dead code

@pabadm
Copy link
Author

pabadm commented Dec 3, 2023

Hello, also apologies for the delay. I recently found out that CSP blocks Function constructor.
So I decided to stick up with your implementation and optimised it a lot using WeakMap (setPrototypeOf called only once per class creation).
Module works mostly as earlier except the bind method. I decided to make an alias to the regular Function's bind because binding of CallableObject returns CallableObject without properties but with its prototype (may be confusing for a lot of people).

UPD: Simplified code with new.target.prototype. Basically I combined the bind and prototype way from medium article. So now it must be both performant and CSP friendly. (Prototype is set only once and bind is used for prototyped function cloning)

Readme is not that necessary for me but you can add me to contributors/collaborators if you want.

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.

2 participants