feat: add web components to fluent ui as a new package and fix text field styling issue #14203
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 53edbb8:
|
Asset size changes
Baseline commit: 40c70519bffb65b73f84a8e7c6332850aecd3c34 (build) |
| @@ -0,0 +1,11 @@ | |||
| module.exports = { | |||
There was a problem hiding this comment.
Longer-term, if this package is going to live in the Fluent UI repo, it would probably be better if it followed the same lint rules as the rest of the packages. However I know that can be a bit contentious and doesn't necessarily have to be resolved right now.
There was a problem hiding this comment.
I'd tend to generally agree, I'm happy to create a follow-up issue to reconcile this, though some of it may be contentious depending on the rules 😆
| @@ -0,0 +1,100 @@ | |||
| { | |||
| "name": "@microsoft/fast-components-msft", | |||
There was a problem hiding this comment.
Is the plan to keep the @microsoft/fast-components-msft name when published from this repo, or rename to @fluentui/web-components (or something else)? I don't think we currently have permissions set up in our release job to publish to @microsoft. Also, as a convention, we typically try to have package folder names match the unscoped part of the package name.
There was a problem hiding this comment.
Currently, per the issue in reference this package has been published already and there are projects which depend on this both internally at Microsoft and externally, so we can't change the name without consequence. The long term plan is to have this live under the fluent namespace, but that will require planning and some coordination. While I agree in principle of having the name match the package, that's really only possible if we want to leverage the fast namespace here; though, I think that would cause more confusion. I'll ping @levithomason as FYI here as we discussed the organization under "web-components" the other day.
There was a problem hiding this comment.
@ecraig12345 I think we need to work on integrating releases. We will likely need to prioritize that discussion, but in the meantime - are we able to release manually within the scope of the package? I have permissions to publish to this package at that namespace.
There was a problem hiding this comment.
Another question here - if we need to publish separately from the current system, do I need to opt out of the change requests to pass the build? If so, is that acceptable to do as part of this PR?
packages/web-components/package.json
Outdated
| "test-firefox:verbose": "karma start karma.conf.js --browsers=FirefoxHeadless --single-run --coverage --reporter=mocha" | ||
| }, | ||
| "devDependencies": { | ||
| "@microsoft/api-extractor": "^7.7.13", |
There was a problem hiding this comment.
Having different versions of build and types dependencies between this package and the rest of the repo is going to cause some headaches:
- We use syncpack to validate that all packages have matching dependency versions (otherwise we're relying on people noticing duplicates in yarn.lock, and things regularly used to sneak through that way and cause problems later). Unfortunately, syncpack currently has no option to ignore a particular package or allow multiple versions for certain deps, so we'll need to either find a different tool, or add this feature to syncpack (original or fork).
- Largely due to the common practice of type packages using
*as the version spec when depending on other type packages, if multiple versions of the same types are installed, it can be unpredictable which one will be picked up - I think having multiple versions of typescript or other dependencies around can also cause problems (don't remember specifics as I haven't had to deal with this for awhile)
In some cases, we may be able to resolve the dependency mismatches without too much trouble, but in other cases it will require work in either web-components or the entire repo to handle a nontrivial upgrade or downgrade.
There was a problem hiding this comment.
I'll try to reconcile these, but one we won't be able to reconcile is Typescript. There are ways to prevent hoisting of certain packages to the root which in the case of Typescript may be beneficial for the short term.
There was a problem hiding this comment.
most of these have been reconciled. I did add a flag to prevent hoisting of the TS related dependencies here, so that should leave everything else undisturbed and install those locally for use in this package: https://classic.yarnpkg.com/blog/2018/02/15/nohoist/
| "main": "dist/esm/index.js", | ||
| "types": "dist/fast-components-msft.d.ts", | ||
| "unpkg": "dist/fast-components-msft.min.js", | ||
| "scripts": { |
There was a problem hiding this comment.
For this package to build properly with the rest of the repo, there may be some modifications needed to the scripts and/or to the CI job.
We use lerna run to run the following scripts in each package:
cleanbuild: core build stuff, usually running TS but not bundling (though this could vary by package)bundle: runs webpack if appropriate (not all packages have this)lint: runs eslintprettier: runs prettier (without--writeoption by default)test: runs tests (mostly unit tests)
If there are any other scripts that you need run as part of the PR/CI job, azure-pipelines.yml will need to be modified to handle that.
There was a problem hiding this comment.
Updated the task to clean from clean:dist and eslint to lint. I've dropped prettier as it doesn't seem to be defined for any of the other packages and running at the root accomplishes what's needed. While we have several tests denoted in here, the primary task is labelled test. Hopefully that reconciles where need be!
chrisdholt
force-pushed
the
users/chhol/add-fast-web-components
branch
from
a338632
to
e4341aa
Compare
| @@ -81,6 +81,12 @@ | |||
| "packages/fluentui/*" | |||
| ] | |||
| }, | |||
| "nohoist": [ | |||
| "packages/web-components/typescript", | |||
| "packages/web-components/tsconfig-path", | |||
There was a problem hiding this comment.
For this one it should be okay to update the version in scripts to match yours.
package.json
Outdated
| "packages/web-components/typescript", | ||
| "packages/web-components/tsconfig-path", | ||
| "packages/web-components/ts-loader", | ||
| "packages/web-components/ts-node" |
There was a problem hiding this comment.
I'm not seeing anywhere obvious that you're using ts-node? Unless it's a peer dep of something.
|
While I appreciate the desire to fully align all deps, linting, etc. I don't think that should be a requirement in the short-term (and for some things, perhaps never). It's important to note that we're building a tech stack from the ground up, with no dependency on React. As such, we have different patterns than would be expressed in React components. A few examples:
|
yarn.lock
Outdated
| ansi-align@^3.0.0: | ||
| version "3.0.0" | ||
| resolved "https://registry.yarnpkg.com/ansi-align/-/ansi-align-3.0.0.tgz#b536b371cf687caaef236c18d3e21fe3797467cb" | ||
| integrity sha512-ZpClVKqXN3RGBmKibdfWzqCY4lnjEuoNzU5T0oEFpfd/z5qJHVarukridD4juLO2FXMiwUQxr9WqQtaYa8XRYw== | ||
| dependencies: | ||
| string-width "^3.0.0" | ||
|
|
||
| ansi-colors@3.2.3: | ||
| ansi-align@^3.0.0, ansi-colors@3.2.3: | ||
| name ansi-align |
There was a problem hiding this comment.
@ecraig12345 If this was a manual change I believe it's broken the repo from being able to be built. I'll need to try and revert (at least) this change. Were there any other manual changes to the lockfile being made as part of this? Is manually modifying the yarn.lock file a common practice? My concern would be the introduction of breaks fairly easily by messing with how yarn resolves and manages dependencies.
There was a problem hiding this comment.
Yeah...that part was a mistake, sorry. Yarn probably warned about it but it got lost in the sea of spurious missing peer dependency warnings. I just pushed a fix. (The fix process is to delete the invalid entry and run yarn to let it be regenerated.)
To the other part of your question, there were quite a few manual changes because yarn tends to resolve new dependencies in a way that results in often-unnecessary duplicates (like we saw with babel and storybook), and I'm not sure how to completely work around it without manual changes.
For example, say you have some existing (probably indirect) dependency on foo@^1.1.0 which yarn resolved to foo@1.1.0 and put in the lockfile. Then something else adds a dependency (again, probably indirect) on foo@^1.2.0 . Instead of changing both to resolve to foo@1.2.0, which satisfies both semver specs, it adds a separate entry/resolution just for the new spec. (IIRC this can even happen with older semver specs: a new indirect dep on foo@^1.0.0 will also result in a separate resolution being added to the newer foo@1.2.0.)
I'm pretty sure this is an intentional behavior done for safety purposes (at least avoiding the implicit upgrade of the original foo@^1.1.0 to foo@1.2.0...adding a separate resolution to a newer version for foo@^1.0.0 just seems weird) but is less than ideal from an install time/size perspective--especially in cases like babel or storybook stuff that has a bunch of dependencies and you get a cascading effect.
Regarding how to validate manual changes, yarn does warn on blatantly invalid things like this, so I should have looked more closely through the sea of peer dep errors to check for any real issues. Beyond that, generally as long as the repo builds and tests pass afterwards, it's probably fine. Also I try and avoid messing with direct dependencies where version upgrades may be particularly impactful, but we usually try and lock those down to a pretty specific version (~ or exact) to begin with.
There was a problem hiding this comment.
It's probably possible to resolve some of this without manual changes using yarn upgrade or yarn upgrade-interactive, and maybe I should try that first in the future instead. Both take an optional package name or scope, so it may be possible to do something similar to what I was doing manually with yarn upgrade-interactive.
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
Chris and I have been talking about this and resolved or worked around most of the issues for now. There are a few different reasons for wanting dependency alignment where reasonable, primarily revolving around maintainability. (You have some good points about exceptions, which I responded to below.)
Regarding the specific cases you mentioned where not aligning dependencies may make sense:
This is a good point, but I do still think that long-term, it would be nice to align our non-stack-specific rules if possible (definitely not blocking on that now). Having significantly different rules within the same repo could be confusing to contributors, unless there's no overlap between who's contributing to which packages. Though I guess you could also argue that having different rules in the web components package and the main FAST repo would confuse developers moving between the two of those, so maybe the question is where there will be more overlap.
Agreed, this is an area where aligning is not really necessary.
I definitely understand wanting to be able to pick up new features in a way that suits your requirements, as well as sometimes wanting to experiment with new versions in selective packages (could see us wanting that with react components sometimes too). But if we do start sharing code between web component and React packages, any package with shared code will always have to use the "lowest common denominator" TS version to avoid breaks. (There's also the potential for other breaks due to multiple TS versions, which I really wish I remembered the details of better...) |
@ecraig12345 Thanks for the response above. I won't speak to everything there, but I do want to zero in on this specific final paragraph. While I understand the goals of keeping dependencies shared in an effort to minimize conflicts, keep build times down, etc - we can't move forward under the premise of using the "lowest common denominator". Web components have a specific browser matrix which doesn't include many of the down-level browsers which need to be supported by Fluent React. The requirements are fundamentally different and in order to offer the best possible web component solution, we can't build for the lowest common denominator. In many cases, these are browsers which will never have support for custom elements, decorators, css custom properties, etc. Ultimately, to provide the best possible web component experience, I don't think we can have a requirement that we share code with other packages in this repo. I'm totally on board with a goal of shared code, but that can't eclipse the goals of the Web Component package. Again, I totally understand the goals and intent with sharing dependencies and code across the mono repo - but from a core requirement standpoint, the WC's need to be able to stand alone. Ultimately, the web component work can't be held back for the lowest common denominator. |
|
One quick clarification is that the "lowest common denominator" comment only applies to the TS version of any hypothetical future packages which contain code shared between different tech stacks (if/when we reach that point), not really anything about browser support, and wouldn't be a concern if there are never shared-code packages. |
chrisdholt
force-pushed
the
users/chhol/add-fast-web-components
branch
from
2031f78
to
5783b63
Compare
|
@ecraig12345 super odd - but the test failures I saw in master on my mac don't repro when pushing :). Looks like all checks passed if you want to take another look. + @dzearing as FYI. |
|
Woohoo! |
|
Now that the web components have been moved here, I was wondering if there is going to be an in-depth documentation for how to properly use it as with all the other Fluent UI packages. The FAST documentation isn't really as explanatory as Fluent's. |
|
Hey @BrunoBlanes - thanks for this. We're working with the team to coordinate a shared documentation story for these. I'll also write up an issue here as I think we'll need a temporary solution as well. Are there specific things you would like to see prioritized while we work to coordinate the docs story with the existing Fluent solutions? |
|
Yes, it is a bit hard to use JavaScript with Blazor, so it makes it even harder to apply custom styling to components. For example, Fluent UI React allows me to set a custom theme, using the Theme Designer and the const myTheme = createTheme({
palette: {
themePrimary: '#0078d4',
[...]
}});With FAST Design I haven't yet figured out how to do customizations on a general level. Blazor also doesn't support NPM so we have to rely on CDN scripts and I couldn't get it to work with FAST Design using TypeScript. The compiled JS version of Fluent UI's tutorials is much less complex then FAST Design. Also I've been using codepen.io to better understand what I have to do to use Fluent UI in Blazor and the direct demo links just saves a lot of time, those aren't available on FAST Design's documentation. I think improving the Blazor specific documentation of both FAST Design and Fluent UI is very important given how much Blazor is expected to grow and also taking into consideration that there aren't many UI libraries available to use with Blazor right now let alone consistent ones like Fluent. Quick note about using Fluent UI React with Blazor, before I knew about FAST I was trying to create Razor Components for every Fluent UI component I would be using, but I have been advised against it because of possible conflicts between React and Blazor. So FAST is the one and only option to use Fluent in this case that I know of. |
…ield styling issue (microsoft#14203) * feat: add web components to fluent ui as a new package * yarn lockfile * Fix dependency versions and scripts * Exclude web-components from publishing * fix invalid dep * do not hoist types in web components * update repo url to package location Co-authored-by: Elizabeth Craig <elcraig@microsoft.com>
Pull request checklist
closes #14071
$ yarn changeDescription of changes
This PR Closes the RFC to add web components to the repository. A few fixes do accompany this change, but as far as this repo is concerned it is net-new.