microsoft / fast

The adaptive interface system for modern web experiences.
https://www.fast.design
Other
9.23k stars 590 forks source link

rfc: make FAST Element a peer dependency of Foundation, Router, and SSR packages #6908

Open chrisdholt opened 7 months ago

chrisdholt commented 7 months ago

💬 RFC

🔦 Context

Currently the Foundation, Router, and SSR packages are tightly coupled to pinned versions of FAST Element. This can make upgrading certain versions of packages difficult from a consumer perspective, especially if certain versions are in conflict. While some package managers do a good job of resolving these deltas, it seems like setting these under "peerDependencies" will provide a better overall and more manageable experience. I believe most package managers will install these by default if not included, but it also seems quite reasonable to assume that if you're using these in your project, you're using FASTElement. The peer dependencies would reflect the most recent major version of FAST Element.

Outstanding questions:

rajsite commented 7 months ago

I think that would make sense if it is necessary to function. If someone pulls in components from two different component packages that pinned to different foundation versions would that break or just increase the size of the package?

If it breaks then it should definitely be a peer dependency (and I'd love to see what changes could be made to undo that requirement) if it doesn't and multiple copies of fast-element and fast-foundation can be compiled into the same app then I'd leave it to a tooling optimization (overrides / yarn flat to figure out de-duplicating for bundle size, etc).

I think peerDependencies shouldn't be used to wedge in an optimization (forcing only a single copy of a library) but to prevent breakages (having multiple copies of the library will actually break your app).

rajsite commented 7 months ago

Alternate wording of my previous comment:

Forcing peerDependencies when it is not needed causes more potential headache for me. I have to resolve the peerDependency by fixing up package versions in order to ship (or choose too force overrides just to get things too build). Tends to break tooling like rennovate / dependabot on automatic upgrades. I don't have the option to ship a bulkier app temporarily that is still functional.

Missing peerDependencies when they are needed makes it easy for my app too get in a bad state. Globals may get overridden when two copies of the same library get compiled in and break each other in subtle ways. I have to manually track that I don't include multiple copies of the same library accidentally.

chrisdholt commented 6 months ago

Thanks for the comments @rajsite - by scoping specifically to FAST Element my hope is to remove headaches (and heartaches). Ultimately, if you're building with FAST Foundation, or using FAST SSR or Router, you've defined a version of FAST Element in your library or application. The goal here is to tell the lowest level and most fundamental portion of the library to use the version you have defined. IMO, this prevents duplication or conflicts and enables upgrading that core library only when you are ready or need to. With the current state of affairs, the library calls for set versions, so if you don't update FE along with Foundation, you're in resolution mode.

I think at this level, peer dependencies prevent the bloat by deferring resolution of FAST Element to your enumerated version rather than requiring resolution every update of a package we publish enumerating it as a dependency.

On the missing peer deps standpoint, the latest versions of both yarn and npm both will install non-enumerated peer dependencies by default. I believe pnpm would do the same.

All said, the intent here is to allow you to define the core library version you're building on and prevent mismatched dependencies.

rajsite commented 6 months ago

A more concrete example, say my app has the following dependencies:

Without peerDeps, the following builds fine (assuming multiple copies of fe won't break my app):

fluent {
  deps: { fe: ^4}
}
other {
  deps { fe: ^3}
}

With peerDeps, npm install errors. The packages need to be updated to install cleanly or I need to force overrides:

fluent {
  peerDeps: { fe: ^4 }
}
other {
  peerDeps { fe: ^3}
}

This causes heartache. Using peerDeps should only be done if multiple copies are actually bad for the app. Like an app bringing in multiple copies of React or jQuery is generally breaking. Unexpected things break.

If you bring in multiple copies of FE and FF do things break? 😮

rajsite commented 6 months ago

Is there a reason FF, et. al, in main is not using more normal semver specifiers like the archives branch instead of pinned?

https://github.com/microsoft/fast/blob/c64110d2f1dfbc24a0978c362bcf72b8958e6d73/packages/web-components/fast-foundation/package.json#L98

Is it an artifact of the prerelease tags that goes away in release?

rajsite commented 6 months ago

Actually I'll concede. It's unlikely most apps will need to leverage fast element / fast foundation based components from multiple disparate component library implementations.

I'm not aware of any other fast foundation-like libraries being shared in the community (though we would like to make one soon with menu-button, date-picker, and a few more not in fast-foundation yet).

So at least today you are unlikely to get stuck being forced to override peerDeps between libraries.

Though I will ask in response to:

If configurable, should we do this for current Beta (allowable range with >2.0.0-beta as value)

Is there a reason FF, et. al, in main is not using more normal semver specifiers like the archives branch instead of pinned?

https://github.com/microsoft/fast/blob/c64110d2f1dfbc24a0978c362bcf72b8958e6d73/packages/web-components/fast-foundation/package.json#L98

Is it an artifact of the prerelease tags that goes away in release?

chrisdholt commented 6 months ago

Actually I'll concede. It's unlikely most apps will need to leverage fast element / fast foundation based components from multiple disparate component library implementations.

I'm not aware of any other fast foundation-like libraries being shared in the community (though we would like to make one soon with menu-button, date-picker, and a few more not in fast-foundation yet).

So at least today you are unlikely to get stuck being forced to override peerDeps between libraries.

Though I will ask in response to:

If configurable, should we do this for current Beta (allowable range with >2.0.0-beta as value)

Is there a reason FF, et. al, in main is not using more normal semver specifiers like the archives branch instead of pinned?

https://github.com/microsoft/fast/blob/c64110d2f1dfbc24a0978c362bcf72b8958e6d73/packages/web-components/fast-foundation/package.json#L98

Is it an artifact of the prerelease tags that goes away in release?

This is likely just an artifact of publishing.

@rajsite agreed on this comment around common usage. Out of curiosity, what monorepo solution are you all using? I know that some have options to prevent hoisting certain packages where there are deltas. I'd like to dig in more if there is a concern this will cause headaches broadly. My guess is that "generally" it's beneficial, but folks hosting disparate versions may need to signal to not hoist different major versions and would want to be sure the major players account for that.

bheston commented 6 months ago

I'm not aware of any other fast foundation-like libraries being shared in the community (though we would like to make one soon with menu-button, date-picker, and a few more not in fast-foundation yet).

We should get those into fast-foundation! I was planning to take over the abandoned date picker once I was done with the other foundation updates I have in queue. Would love to see more contributions to flesh out the offering.