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

Remove react as peer dependency #386

Closed
christian-bromann opened this issue Aug 2, 2022 · 8 comments
Closed

Remove react as peer dependency #386

christian-bromann opened this issue Aug 2, 2022 · 8 comments
Assignees
Labels
enhancement New feature or request

Comments

@christian-bromann
Copy link

Describe the bug

As described in the readme:

Tech stacks: The library ships as a set of web components. This means developers can use the toolkit no matter which tech stack – React, Vue, Svelte, etc. – their extension is built with.

However react is explicitly defined as peer dependency which causes warnings during install.

To reproduce

Run yarn add @vscode/extension-telemetry

❯ yarn add @vscode/extension-telemetry
yarn add v1.22.18
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
warning " > @vscode/webview-ui-toolkit@1.0.0" has unmet peer dependency "react@>=16.9.0".
warning "@vscode/webview-ui-toolkit > @microsoft/fast-react-wrapper@0.1.48" has unmet peer dependency "react@>=16.9.0".
[4/4] 🔨  Building fresh packages...
success Saved lockfile.
success Saved 5 new dependencies.
info Direct dependencies
└─ @vscode/extension-telemetry@0.6.2
info All dependencies
├─ @microsoft/1ds-core-js@3.2.4
├─ @microsoft/1ds-post-js@3.2.4
├─ @microsoft/applicationinsights-core-js@2.8.5
├─ @microsoft/applicationinsights-shims@2.0.1
└─ @vscode/extension-telemetry@0.6.2
✨  Done in 6.58s.

Expected behavior

There is no peer dependency defined. Even though it is only a warning and it works nonetheless, I suggest to remove it as it suggests something wrong.

Current behavior

Peer dependency is defined and causes warnings during install.

Screenshots

Desktop (please complete the following information):

  • OS Version: [e.g. macOS 11.3.1]
  • Toolkit Version: [e.g. v0.8.0]

Additional context

@christian-bromann christian-bromann added the bug Something isn't working label Aug 2, 2022
@daviddossett
Copy link
Collaborator

FYI @hawkticehurst is on leave at the moment—I'll wait for him to comment here as I'm pretty sure this was a trade off we had to make for various reasons.

@hawkticehurst
Copy link
Member

Howdy, I'm back and am happy to talk about this!

This is definitely one of the weirder parts of the toolkit right now... but I am in agreement that this peer dependency is less than ideal (and has personally always bugged me).

React doesn't natively support web components 🙃😪

The short version is that React (as you may or may not know) does not natively support web components at this time. It means we have to wrap all the web components in React components and export those for React devs to use.

In our case, we use a package from the FAST team to handle this wrapping code and it results in this single file where a React import is needed (and thus the peer dependency).

There is hope that native web component support will finally arrive in React v19 and this reality might be able to change a bit, but it's still a little unclear when that will land.

An alternative?

In the meantime (and in light of this issue), I have been doing a bit of snooping around and looking at other web component libraries to see how they handle React support and have discovered there might be another away to support React devs while being able to remove the React peer dep.

I still need to do more research but am happy to allocate a short sprint to this topic and see what's possible sometime in the next few weeks.

@hawkticehurst
Copy link
Member

Hope that gives a little bit more insight into this topic, but definitely let me know if you have any follow up questions/concerns.

@christian-bromann
Copy link
Author

Thanks for the input. It's not blocking me in anyway, just wanted to have it noticed by the team.

@hawkticehurst
Copy link
Member

Ahh I see, in that case, yes it's definitely on our radar.

@svew
Copy link

svew commented Oct 19, 2022

Any updates on this? Not blocking, but annoying :)

@hawkticehurst
Copy link
Member

Unfortunately, I don't have any notable updates. I did some explorations on the "alternative" I was hopeful about and it proved to be not super fruitful for our particular scenario/architecture

Since that time we have become really tightly resourced (myself and one other person split our time between this project and a handful of others) so we probably won't be able to tackle it for a while since it's a non-blocking issue

As I find pockets of time, however, I'll definitely try to revisit this and give updates on anything I discover

@hawkticehurst hawkticehurst added enhancement New feature or request and removed bug Something isn't working labels Dec 13, 2022
@hawkticehurst
Copy link
Member

hawkticehurst commented Apr 14, 2023

Hey all! As of PR #470, this issue is now resolved by separating the main toolkit components and the toolkit react components into separate NPM packages :)

It's part of vnext work that should be published by the end of June '23 when Toolkit V2 gets released. But there's a good chance I'll publish beta packages for those who might want to take advantage of this sooner than later, so I'll circle back when that happens

I'm also going to go ahead and close this issue now to more closely track when this work was completed, however, please feel free to continue posting here with any questions/comments and I'll keep an eye on this thread

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants