Converting to Typescript: first draft

[vicb]

@realiarthur
This PR is a work in progress to get feedback.
Let me know if you're happy with the updates here and the plan.
Thanks !

Done:

• Conversion to typescript - still needs validation but ok for a preview,
• Fixed a bug in withValue.js - disconectedCallback -> disconnectedCallback (nn)
• Fixed a bug in utils.js - the parsed variable was in the global namespace
• Fixed a bug in withForm.js handleBlur() - missing return type. Fixed by removing recursion.

ToDo:

• regression on the minified size. Was 5.49 kB before this update. Probably related to terser config,
• add a TS version of the example,
• add some inline doc on types,
• update the doc,
• Add a changelog with breaking changes (polyfill, API) & other changes
• mention the Element.closest polyfill in the docs,
• Build cjs, esm, umd, and .d.ts - make sure umd bundle has the same path as before,
• Add ESLint to format the code (as a last commit as it will make review harder).

[vicb]

I have played a little more with that.

My conclusion is that some of the construct do not play well with typescript:

• the closest polyfill,
• the fact that some methods take nameOrEvent - handleBlur and handleChange,
• there is probably no need have the option to pass options to withForm nor withField

In the end I feel like converting to TS would only make sense with a few breaking changes. So I am wondering if it should be done in this repo (with a new major release) or if it is better done in a fork.

What do you think ?

[realiarthur]

- As I said my point about closest polyfill is that builtin function may works faster. But if there is no other way, we can make a js function - handleChange and handleBlur is more like inner functions, so we can do make whatever we want with this. But somehow formik support both typescript and arguments like nameOrEvent (at least version 1.x.x) - why do not pass options to withForm? I’m always use withForm with options like “validationSchema” or “onSubmit”. It looks like standard HOC pattern and make code more readable. Or i’m wrong? If you don't want to deal with it further, of course you can fork it and do it like you want)

…

On Mon, 2 Nov 2020 at 06:37, Victor Berchet ***@***.***> wrote: I have played a little more with that. My conclusion is that some of the construct do not play well with typescript: - the closest polyfill <https://github.com/realiarthur/lite-form/blob/9a7305361d8daa774fa76d97c3c1da1cdb43a852/src/utils.js#L34> , - the fact that some methods take nameOrEvent - handleBlur <https://github.com/realiarthur/lite-form/blob/9a7305361d8daa774fa76d97c3c1da1cdb43a852/src/withForm.js#L122> and handleChange <https://github.com/realiarthur/lite-form/blob/9a7305361d8daa774fa76d97c3c1da1cdb43a852/src/withForm.js#L144> , - there is probably no need have the option to pass options to withForm <https://github.com/realiarthur/lite-form/blob/9a7305361d8daa774fa76d97c3c1da1cdb43a852/src/withForm.js#L16> nor withField <https://github.com/realiarthur/lite-form/blob/9a7305361d8daa774fa76d97c3c1da1cdb43a852/src/withField.js#L22> In the end I feel like converting to TS would only make sense with a few breaking changes. So I am wondering if it should be done in this repo (with a new major release) or if it is better done in a fork. What do you think ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGKWTPI7AH2ZVY4UUHZ55I3SNYZQLANCNFSM4TGD7IAA> .

[vicb]

• As I said my point about closest polyfill is that builtin function may works faster. But if there is no other way, we can make a js function

The JS function would be actually be the same as the polyfill, just not on the prototype.

• handleChange and handleBlur is more like inner functions, so we can do make whatever we want with this. But somehow formik support both typescript and arguments like nameOrEvent (at least version 1.x.x)

It's probably possible to make a working typing with some effort.
Is it worth it ? I don't think so. We know which or name or event we will pass so having different function would make things clearer (and simpler) IMO.
On the other it's a breaking change if someone rely on it.

• why do not pass options to withForm? I’m always use withForm with options like “validationSchema” or “onSubmit”. It looks like standard HOC pattern and make code more readable. Or i’m wrong? The way I use it is to implement validationSchema or onSubmit in my Lit Element class.

With TS, you have to use class MyComponent extends withForm(LitElement) {...} to be able to reference handle... anyway so passing options as little sense here.

Dropping the options would allow to simplify the logic (i.e. no more need for withFormExtended and simplify this._onSubmit = (onSubmit || this.onSubmit || function () {}).bind(this)). But if there are use cases no need to remove it.

If you don't want to deal with it further, of course you can fork it and do it like you want

I think forking should be last resort. It would be better if can discuss/agree on how to evolve this lib.

[realiarthur]

I agree with your points #1 and #2, it not make a huge sense. But it really useful for me to pass options to withForm and withField. So, if we can make version 2 with this agreement it will be excellent!

…

On Tue, 3 Nov 2020 at 22:10, Victor Berchet ***@***.***> wrote: - As I said my point about closest polyfill is that builtin function may works faster. But if there is no other way, we can make a js function The JS function would be actually be the same as the polyfill, just not on the prototype. - handleChange and handleBlur is more like inner functions, so we can do make whatever we want with this. But somehow formik support both typescript and arguments like nameOrEvent (at least version 1.x.x) It's probably possible to make a working typing with some effort. Is it worth it ? I don't think so. We know which or name or event we will pass so having different function would make things clearer (and simpler) IMO. On the other it's a breaking change if someone rely on it. - why do not pass options to withForm? I’m always use withForm with options like “validationSchema” or “onSubmit”. It looks like standard HOC pattern and make code more readable. Or i’m wrong? The way I use it is to implement validationSchema or onSubmit in my Lit Element class. With TS, you have to use class MyComponent extends withForm(LitElement) {...} to be able to reference handle... anyway so passing options as little sense here. Dropping the options would allow to simplify the logic (i.e. no more need for withFormExtended and simplify this._onSubmit = (onSubmit || this.onSubmit || function () {}).bind(this)). But if there are use cases no need to remove it. If you don't want to deal with it further, of course you can fork it and do it like you want I think forking should be last resort. It would be better if can discuss/agree on how to evolve this lib. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGKWTPKRS7TQ3E2HTYBX3L3SOBPU5ANCNFSM4TGD7IAA> .

[vicb]

Great.
Let's recap

For 1) I am not sure if you want to keep the closest() code without patching on Element.proto or if the code should be dropped and users should explicitly load a polyfill (the latter is a breaking change).

2. methods that accept nameOrElement are split in 2 different methods each - this is a breaking change.

3. we keep the ability to pass options to withForm and withField.

We need to clarify 1 - and make sure my understanding of 2 and 3 is correct.

[realiarthur]

For 1 I think you are right, we can drop it, and make users responsible about ie. For 2 and 3 yes, all is like you write Thank you for discuss!

…

On Wed, 4 Nov 2020 at 19:29, Victor Berchet ***@***.***> wrote: Great. Let's recap For 1) I am not sure if you want to keep the closest() code without patching on Element.proto or if the code should be dropped and users should explicitly load a polyfill (the latter is a breaking change). 1. methods that accept nameOrElement are split in 2 different methods each - this is a breaking change. 2. we keep the ability to pass options to withForm and withField. We need to clarify 1 - and make sure my understanding of 2 and 3 is correct. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGKWTPO2URH2R3IVTPOSWPTSOGFQNANCNFSM4TGD7IAA> .

[vicb]

Hey,

Could you please take a look at the updates here ?

[vicb]

Thanks for your review.

I have added some suggestion in withForm they should also be applicable to withField.
That will be breaking changes.
It is also possible to not include those breaking change and keep the former behavior.
Let me know what you prefer.

As noted in the comments the current setup does not bundle lodash (to stay consistent with your setup). Bundling lodash increases the bundle size by a lot. It should be possible to come with implementation of get, set and cloneDeep that are much lighter. I don't think this should be a priority of this CL though. There are probably libs on npm.

[realiarthur]

Thank you! Please see few my comments

[vicb]

I have implemented the changes we discussed.

I'll update the description of this PR to list the remaining tasks.

[realiarthur]

Thank you! Please see few my comments

[vicb]

@realiarthur Thanks for the great feedback.

I have integrated your feedback.

I'll work on the remaining tasks later today.

[vicb]

Quick update on this CL: I need to be able to block some time to finish the remaining tasks here. I will probably not be able to dedicate this time before December. Do not expect any big process in the next 2 weeks.
Thanks.

[realiarthur]

@vicb I plan to merge your PR and finish the remaining (build and documentation) tasks in the "2.0" branch. You do not mind?

[vicb]

Oh sorry I completely forgot about this PR.

I'll be out for a week and can give a hand after that.