Skip to content
This repository has been archived by the owner on Jan 6, 2025. It is now read-only.

feat: expose wrapped (Fast) React components #300

Merged
merged 34 commits into from
Jan 25, 2022
Merged

feat: expose wrapped (Fast) React components #300

merged 34 commits into from
Jan 25, 2022

Conversation

mattrothenberg
Copy link
Contributor

@mattrothenberg mattrothenberg commented Nov 17, 2021

Link to relevant issue

This pull request resolves #299

Description of changes

This PR adds the ability for users to import wrapped React components directly from a subdirectory of the @vscode/webview-ui-toolkit package thusly:

import { VSCodeButton, VSCodeCheckbox } from '@vscode/webview-ui-toolkit/react'

Specifically, this PR:

Adds a src/react/index.ts file which exports wrapped versions of all of the components in this library

import React from 'react';
import {provideReactWrapper} from '@microsoft/fast-react-wrapper';
import {provideFASTDesignSystem} from '@microsoft/fast-components';

import { vsCodeBadge } from '..'

const {wrap} = provideReactWrapper(React, provideFASTDesignSystem());

export const VSCodeBadge = wrap(vsCodeBadge());
// ETCETERA

Adds a tsconfig-react.json and (supporting NPM script) to package up the aforementioned file.

// package.json
"build:react": "tsc -p ./tsconfig-react.json",

Proposes a modification to our build process, such that users can import directly from the react subdirectory. 🚨 Please note that this requires a significant change to the build process. 🚨

Specifically, the steps are:

  1. After building both the regular components and the React wrapped components, copying a stub version of the package.json file to the /dist directory. This stub file is a mirror image of the original package.json with the exception of the values in the main and types fields losing their /dist prefix.
    2.cding into the /dist directory and running publish from within there. This ensures that the built files that end up being distributed on NPM have a flat structure (rather than a nested dist directory) 👇

Old, Nested Directory structure

@vscode/webview-ui-toolkit/
├─ dist/
│  ├─ esm/
│  ├─ dst/
├─ package.json

In this old world, the main file was /dist/esm/index.js.

New, Flattened Directory structure

@vscode/webview-ui-toolkit/
├─ package.json
├─ react/
├─ dst/
├─ esm/

In this new world, the main file is /esm/index.js, but users can also import directly from react/index.js.

There are likely less kludgy ways of doing this, but this seems to work 🤷 .

Finally, I'd like to follow-up this PR by building a Storybook dedicated solely to the wrapped React components. As far as I an tell, it's extremely difficult (if not impossible) to use multiple frameworks within a single Storybook instance. It is possible, however, to "compose" storybooks via the following API https://storybook.js.org/docs/react/workflows/storybook-composition. To that end, once this PR is merged and once I have the other Storybook set up, I will update the Storybook here to pull in the published React SB instance.

@ghost
Copy link

ghost commented Nov 17, 2021

CLA assistant check
All CLA requirements met.

@mattrothenberg mattrothenberg marked this pull request as ready for review November 19, 2021 14:53
@mattrothenberg
Copy link
Contributor Author

cc @hawkticehurst ! I wasn't able to add you as a reviewer

package.json Outdated Show resolved Hide resolved
Copy link
Member

@hawkticehurst hawkticehurst left a comment

Choose a reason for hiding this comment

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

Finally had a free moment to take a closer pass on this PR and it's looking great!

Left a couple of comments asking for changes and some minor nits when you're back in the office, but super excited with the progress! Thank you again for the help.

src/react/index.ts Outdated Show resolved Hide resolved
src/react/index.ts Outdated Show resolved Hide resolved
src/react/index.ts Outdated Show resolved Hide resolved
src/react/index.ts Outdated Show resolved Hide resolved
tsconfig-react.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
copy-pkg-json.js Outdated Show resolved Hide resolved
@mattrothenberg
Copy link
Contributor Author

@hawkticehurst All feedback addressed! I've tested that the build works locally, though I'm confused about one thing.

I've been testing locally with a tool called yalc by doing the following:

  • Running npm run build
  • cding into the dist directory
  • Running yalc publish

(You'll recall that in this new world, we have to publish from within the dist directory otherwise we won't get the subdirectory imports. I wish there was a better way of doing this 🤔 )

Unfortunately the latter yalc publish command consistently fails when the prepublishOnly script is present in the project's package.json, since that script triggers the build command again and fails to find the rollup.config.js file (since we're in dist at this point, not root).

Any thoughts here?

@hawkticehurst
Copy link
Member

Any thoughts here?

Hmm, it's probably just a case of removing the prepublishOnly script for the time being (with the assumption that at some point TypeScript will eventually ship the feature that would make exporting subdirectory type info way easier) and manually run npm run build before I publish. I only had the pre-publish script as a contingency in case I ever forgot to build before publishing but I've done pretty well so far.

With that said, I could envision potentially doing something fancy with the npm --prefix flag, so I can do some experiments today. And in the meantime, I think removing the prepublishOnly script is fine (assuming that fixes the problem).

@hawkticehurst
Copy link
Member

(You'll recall that in this new world, we have to publish from within the dist directory otherwise we won't get the subdirectory imports. I wish there was a better way of doing this 🤔 )

Lol thanks for the reminder, I had forgotten about that. But yeah we can manage for now and hopefully the ability to export subdirectory type info will land sooner versus later 😪.

Copy link
Member

@hawkticehurst hawkticehurst left a comment

Choose a reason for hiding this comment

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

Changes look and function great! Thank you!

While reviewing I did catch two more things that I left comments on.

src/react/index.ts Outdated Show resolved Hide resolved
src/react/index.ts Outdated Show resolved Hide resolved
@hawkticehurst hawkticehurst assigned clocky and unassigned clocky Dec 8, 2021
@hawkticehurst hawkticehurst added this to the January 2022 milestone Dec 8, 2021
@hawkticehurst hawkticehurst added the enhancement New feature or request label Dec 8, 2021
@pwang347
Copy link
Member

@hawkticehurst Hi Hawk! I read through the discussions in the fast repo, and was just wondering if there's an ETA on getting the stop-gap solution merged. We're currently hoping to adopt the toolkit (in React) to try it out as early as possible.

@hawkticehurst
Copy link
Member

@pwang347 Hi Paul! I'd wager publishing a set of React components (i.e. this PR) is probably still two or so weeks out.

However, if you're really eager and can't wait to get up and running I could decidedly have the stop-gap solution mentioned in those issue threads published first thing next week, which would unblock the ability for toolkit users to wrap the web components themselves (I would, of course, give some guidance on how to do that properly).

Then once this PR lands (which will do the job of wrapping components for toolkit users) you could simply remove your wrapping code and switch to importing React components directly.

Lemme know if that's something you would be interested in or if you'd rather wait for this PR. :)

@pwang347
Copy link
Member

@hawkticehurst Sounds good! Yes, I think even with the added manual wrapping, having the stopgap solution in the meantime to unblock would be great (if it's not too much trouble for you). Appreciate the fast response, thanks!

@hawkticehurst
Copy link
Member

hawkticehurst commented Jan 5, 2022

@hawkticehurst Sounds good! Yes, I think even with the added manual wrapping, having the stopgap solution in the meantime to unblock would be great (if it's not too much trouble for you). Appreciate the fast response, thanks!

@pwang347 the temporary solution has been published and a quick summary/guide on the update has been posted here (decided to put it in a thread with a higher visibility conversation about the ongoing React work).

Hope that helps unblock you and feel free to open GH issues with any questions or problems you run into!

@pwang347
Copy link
Member

pwang347 commented Jan 5, 2022

@hawkticehurst this is great, will give it a try today or tomorrow! thanks again!

@mattrothenberg
Copy link
Contributor Author

Latest changes from main have been merged in 👍

@hawkticehurst
Copy link
Member

@hawkticehurst this is great, will give it a try today or tomorrow! thanks again!

Great, let us know how it goes!

@pwang347
Copy link
Member

pwang347 commented Jan 6, 2022

@hawkticehurst this is great, will give it a try today or tomorrow! thanks again!

Great, let us know how it goes!

Gave it a try, and so far everything seems to be working great (plugged it into the app, events were working and things felt good)! Only issue I ran into was microsoft/fast#5198, but was able to resolve it with the skipLibCheck workaround.

@hawkticehurst
Copy link
Member

Gave it a try, and so far everything seems to be working great (plugged it into the app, events were working and things felt good)! Only issue I ran into was microsoft/fast#5198, but was able to resolve it with the skipLibCheck workaround.

That's great to hear! Also wasn't aware of this issue so thanks for mentioning it/giving a heads up!

@hawkticehurst
Copy link
Member

Also wasn't aware of this issue so thanks for mentioning it/giving a heads up!

After reading through, I realize I was actually aware of this so thank you for the reminder!

@hawkticehurst
Copy link
Member

hawkticehurst commented Jan 6, 2022

And just to be super clear in case others run into this problem--you got this issue because you are using a TS version >4.3.5?

@pwang347
Copy link
Member

And just to be super clear in case others run into this problem--you got this issue because you are using a TS version >4.3.5?

Apologies for the late reply, yes I am using 4.4.4!

Copy link
Member

@hawkticehurst hawkticehurst left a comment

Choose a reason for hiding this comment

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

Looks fantastic, left some notes/requests for final polish but I think we're basically there!

scripts/copy-pkg-json.js Outdated Show resolved Hide resolved
scripts/copy-pkg-json.js Outdated Show resolved Hide resolved
src/react/index.ts Outdated Show resolved Hide resolved
src/react/index.ts Outdated Show resolved Hide resolved
@hawkticehurst
Copy link
Member

Incredible work @mattrothenberg! Thank you again for helping make this a reality!

@hawkticehurst hawkticehurst merged commit cc58b83 into microsoft:main Jan 25, 2022
@hawkticehurst
Copy link
Member

@pwang347 I'm sure you probably saw already, but in case you didn't as of v0.9.0 we finally shipped the React components if you would like to use them.

You should be able to remove the file that wraps components (and uninstall the fast-react-wrapper package) and instead replace it with import statements along these lines and it should just work:

import { VSCodeButton, VSCodeDropdown } from '@vscode/webview-ui-toolkit/react';

Let me know if you have any questions!

@hawkticehurst hawkticehurst deleted the mr/wrapped-react-components branch January 26, 2022 21:49
@pwang347
Copy link
Member

pwang347 commented Jan 28, 2022

@hawkticehurst @mattrothenberg This is great, thank you!

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

Successfully merging this pull request may close these issues.

Publish toolkit components as wrapped React components
4 participants