Skip to content

Conversation

janebitovi
Copy link
Contributor

@janebitovi janebitovi commented Aug 4, 2022

for: (R2WC-19)
for: (R2WC-17)
for: (R2WC-20)
for: #13

Passing the string to new Function would be useless in most cases (since there's no local context) and would be referencing global functions internally to do anything useful...
So I skipped the eval and use the attr value to directly check for a function on window (or global) (if they exist) and pass that value instead.

Sucks that it has to be on global buuut it's still very useful if needed on the underlying react component.

Thoughts?

New functionality documented here:
https://github.com/bitovi/react-to-webcomponent/blob/a7bf224cd5406d1cdd50a4397e514c6042173fcc/docs/api.md

…l functions and pass the fn to the underlying react component instead
@janebitovi
Copy link
Contributor Author

Currently we only use the keys on propTypes (and they're necessary for now)

If we move forward with https://bitovi.slack.com/archives/C031D2P4SUA/p1659636864346109 though, then maybe instead of merging this as-is,
(we're assuming it's a global function reference based on the attribute beginning with on- or handle-)
we can take a closer look at the values on propTypes and do make the assumption for any prop that's a function type automatically regardless of the name, and, similarly, per the proposal, we'll be allowing the consuming user to specify that it's a function reference when propTypes are not used in the underlying react component.

tl;dr:
don't merge this as is IF the linked proposal is approved, if it's not, then this is still independently ready for review/discussion

@christopherjbaker
Copy link

Please only do this for on-, not for handle-; on- is the convention and is the only style in use by official elements, mimicking DOM usage too.

As to the functionality: Limited as it may be, I feel like it would be ideal if it behaved exactly as regular dom events do. My thought was that we could pass the input into new Function and predefined what arguments they can use (dom uses event only afaik). Something like new Function('event', value), then they could set on-click="console.log(event)" and it would work just like any other element. More importantly, this allows them to control how that global function is called: on-click="doThing("hello", event.value)", something they lose if all they can specify is the name of the function.

@janebitovi
Copy link
Contributor Author

janebitovi commented Aug 5, 2022

Re: dropping handle-
The proposal we're discussing in slack would also have us drop on- b/c type specifications would say when to use function handoff behavior (so any param can be a function instead of relying on hacky sentinel values) which is way better.
(fwiw: I added handle- because the react docs use handlePrefixedCBFunctions in their examples)


Re: Using custom events in the translation layer
Yeahhhh it's not uncommon to curry on the fly like onData={cbParam => myFn("curry", cbParam)} but in our case myFn would already have to be specially placed on window... So either way, the webcomponents dev is forced to dump a function to window to do anything useful...

The added indirection and js engine's inability to optimize eval'd functions makes everything slower too... Plus the implementation on our side of using events just for the translation layer -- then making sure the event is in scope of the callback... you're adding a ton of overhead and gross code to maintain.
I'm not sure the tradeoff is a gain here...

BUT that did give me an idea to improve something:
we should call the global cb function with this set to the web component instance so their global function at least has the opportunity to map updates back to the correct parent when there are multiple instances.


Let's hop on a call asap to hash out the plans! :)

@janebitovi janebitovi marked this pull request as draft August 5, 2022 15:08
…tion, bind Function params to the webcomponent instance
@janebitovi janebitovi marked this pull request as ready for review August 5, 2022 20:19
@janebitovi janebitovi changed the title Allow 'on-*' and 'handle-*' attrs to reference global functions and pass the fn to the underlying react component instead Allow specifying prop types with automatic type casting from attr value string into the underlying React component. Remove propTypes requirement. Aug 5, 2022
@janebitovi
Copy link
Contributor Author

janebitovi commented Aug 5, 2022

New functionality documented here:
https://github.com/bitovi/react-to-webcomponent/blob/a7bf224cd5406d1cdd50a4397e514c6042173fcc/docs/api.md

also removed dashStyleAttributes and left room for a future option for users to specify their own camelCasedProp -> kebab-attr mapping per Justin's request.

@luantrasel
Copy link

Was this already merged and released? I'm using v1.7.2 and it's not working

christopherjbaker pushed a commit that referenced this pull request Apr 3, 2023
* begin convert to ts (#49)

* begin convert to ts

* get rid of npmignore because we added files to package json

* wip

* wip too

* simplify tsconfig include path

* r2wc11: support preact v10 (#51)

* Allow specifying prop types with automatic type casting from attr value string into the underlying React component. Remove propTypes requirement. (#50)

* for: (R2WC-19) - allow 'on-*' and 'handle-*' attrs to reference global functions and pass the fn to the underlying react component instead

* for: (R2WC-17) remove propTypes dep, add attr to prop type casting option, bind Function params to the webcomponent instance

* for: (R2WC-20) allow React ref prop types

* for: (R2WC-22) upgrade tests to allow waiting until it's ready without arbirary setTimeout durations and enable useful console logging when they fail since vitest (jest) expectations can't have notes attached normally (#53)

* for: (R2WC-23) use functional components in api docs and ensure they work with tests (#54)

* adding upgraded publish.yml for prereleases (#55)

* Publish v2.0.0-alpha.0

* support children prop (#57)

* support children prop

* fix attributes and preact test

* Update react-to-webcomponent.test.jsx

Co-authored-by: Christopher Baker <christopher@hmudesign.com>

* Bump json5 from 2.2.1 to 2.2.3 (#64)

Bumps [json5](https://github.com/json5/json5) from 2.2.1 to 2.2.3.
- [Release notes](https://github.com/json5/json5/releases)
- [Changelog](https://github.com/json5/json5/blob/main/CHANGELOG.md)
- [Commits](json5/json5@v2.2.1...v2.2.3)

---
updated-dependencies:
- dependency-name: json5
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump minimist and http-server (#70)

Bumps [minimist](https://github.com/minimistjs/minimist) to 1.2.6 and updates ancestor dependency [http-server](https://github.com/http-party/http-server). These dependencies need to be updated together.


Updates `minimist` from 0.0.10 to 1.2.6
- [Release notes](https://github.com/minimistjs/minimist/releases)
- [Changelog](https://github.com/minimistjs/minimist/blob/main/CHANGELOG.md)
- [Commits](minimistjs/minimist@v0.0.10...v1.2.6)

Updates `http-server` from 0.11.2 to 14.1.1
- [Release notes](https://github.com/http-party/http-server/releases)
- [Commits](http-party/http-server@v0.11.2...v14.1.1)

---
updated-dependencies:
- dependency-name: minimist
  dependency-type: indirect
- dependency-name: http-server
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump ecstatic and http-server (#69)

Removes [ecstatic](https://github.com/jfhbrook/node-ecstatic). It's no longer used after updating ancestor dependency [http-server](https://github.com/http-party/http-server). These dependencies need to be updated together.


Removes `ecstatic`

Updates `http-server` from 0.11.2 to 14.1.1
- [Release notes](https://github.com/http-party/http-server/releases)
- [Commits](http-party/http-server@v0.11.2...v14.1.1)

---
updated-dependencies:
- dependency-name: ecstatic
  dependency-type: indirect
- dependency-name: http-server
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update publish.yml

* Update publish.yml

* hot fix for react component prop having same name as an html attribute (#74)

* hot fix for react component prop having same name as an html attribute

* test fix

* improved typing and resolving existing typing issues (#84)

* improved typing

* react and react-dom mock types

* prettier formatting

* slight typing changes

* specific return type for r2wc function

* type support for class components

* moved R2WCOptions to global.d.ts

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Jane <92131113+janebitovi@users.noreply.github.com>
Co-authored-by: Youssef A <youssef@bitovi.com>
Co-authored-by: [bot]@workflow <Workflow: [bot]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Bavin Edwards <65621465+zerico007@users.noreply.github.com>
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.

3 participants