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

[RFC] Package/File Organization for Easier Bundling and Leveraging Future APIs #679

Closed
Westbrook opened this issue May 15, 2020 · 11 comments
Assignees
Labels
enhancement New feature or request

Comments

@Westbrook
Copy link
Contributor

Westbrook commented May 15, 2020

Currently, all of our element packages are structured to leverage index.js (after running tsc) as a side effect full export of all elements included in a package. This includes both the class definitions for the various customs elements that are defined and the side effect of defining them against window.customElements the global (and currently only) element registry. Much of the reasoning for this comes from Justin Fagnani who explicates the belief that this should be so, here: https://justinfagnani.com/2019/11/01/how-to-publish-web-components-to-npm/#always-self-define-elements. This approach is further established by patterns like LitElements @customElement decorator that inherently requires a colocated class definition and define() call. However, as of late, this concept starts to run afoul of some concepts around usability in the future (and possibly at current), performance tuning of custom elements, particularly in cases when we ship a number of elements and/or sub-elements in the same package (e.g. @spectrum-web-components/button), and private API surfacing.

Future Use Cases: Scoped Custom Element Registries

As discussed in WICG/webcomponents#716 the reality of Scoped Custom Element registries may only be in development as yet. However, having been featured in the recent Web Components Face to Face held by the W3C it is highlight likely that this or a close cousin version of this API gets shipped to browsers. While presenting the current state of the API specification at this meeting Fagnani himself admitting to likely needing to revise his best practices suggestion around element registering as part of the specification getting closer to landing. It is certainly possible to leverage a trivial subclass in order to register a custom element definition in both the global and local registries, as is done in the explain like so:

// x-foo.js is an existing custom element module that registers a class
// as 'x-foo' in the global registry.
import {XFoo} from './x-foo.js';

// Create a new registry that inherits from the global registry
const myRegistry = new CustomElementRegistry(window.customElements);

// Define a trivial subclass of XFoo so that we can register it ourselves
class MyFoo extends XFoo {}

// Register it as `my-foo` locally.
myRegistry.define('my-foo', MyFoo);

However, a component package that does this by default would then be leaking details into the global scope as part of the initial registration, which means, in the case of the use case this API prepares us for: multiple registrations of elements with the same name, we could be exposing the possibility of a failed registration or in the case of our registration gating, like so:

if (!customElements.get('sp-action-menu')) {
    customElements.define('sp-action-menu', ActionMenu);
}

The first wins style of registration on which this relies can end up register an incorrect version of the element with little or no notification to the implementing engineer.

Current Use Case: Scoped Element

Teams across the web components community that are already feeling the stresses of multiple registrations and are pushing for standardization around a solution are also pushing user-space solutions to the problem in the interim. Particularly, ING (makers of Lion Elements) in partnership with OpenWC has developed a solution called Scoped Elements that extends lit-html and LitElement to support multiple registrations from a single definition. As we get closer to a fully agreed upon Scoped Registries specification, it is likely that solutions like this will continue to grow in usage and will raise the need to address the issues outlined above sooner.

Tuning for Performance

The difficulties of registering a single element in its entry points have been outlined above, and for a number of our packages, this is multiplied by multiple components being registered in an entry point at a time. Not only does this raise the runtime costs on including a package, but in some cases, it means that a user will need to register multiple components that they won't actually be using in their project in order to get one that they are. This can be keenly felt in @spectrum-web-components/button. Currently, this package delivers three different buttons by default (<sp-button>, <sp-action-button>, and <sp-clear-button>), while the @spectrum-css/button package that it is based on actually describes three more buttons (theoretically, <sp-field-button>, <sp-tool-button>, and <sp-logic-button>) that should at some point be added to the package. Users of <sp-clear-button> the lightest (in KB) are currently required to include the rest due to our current file structure which affects the performance of the applications they are developing. In some cases, they desire to prevent this reality in the context of our current directory structure causes confusion across the project where you see packages like @spectrum-web-components/menu, @spectrum-web-components/menu-group, @spectrum-web-components/menu-item pollute our scope in an effort to avoid forcing an application to define multiple elements by default like you see in @spectrum-web-components/sidenav which breaks down internally to a similarly related family of elements.

Private API Exposure

In packages like @spectrum-web-components/theme and @spectrum-web-components/shared we are actively exposing private API (which might make is the public API) by documenting the use of files inside of the lib folder of our packages. There can be some benefit to doing so across other elements, e.g. doing so will allow users of certain eslint configurations to avoid multiple import messages when they require both the side effects as well as direct access to a class in the same package, a la:

import '@spectrum-web-components/button';
import { ActionButton} from '@spectrum-web-components/button/lib/action-button';

vs

import '@spectrum-web-components/button';
import { ActionButton} from '@spectrum-web-components/button';

However, doing so makes the contents of our lib folders public to our users preventing us from easily making alterations to the contents thereof. While we've not yet encountered a need to do such a thing, the growth of our user bases through more public partnerships with Photoshop and XD will increase the likelihood of this occurring.

Recommendation

To prepare for and/or address the above situations, I'd like to propose the following basic component package directory structure (posts build, so the assets as found after installing the package as a dependency):

- src
  - component-name.css.d.ts // types
  - component-name.css.js // styles for element
  - component-name.css.js.map // source map
  - ComponentName.d.ts // types
  - ComponentName.js // class definition
  - ComponentName.js.map // source map
  - index.d.ts // types
  - index.js // re-exporting of the classes in the package (leverages the `main` and `module` field to prevent from surfacing its location)
  - index.js.map // source map
  - spectrum-component-name.css.d.ts // types
  - spectrum-component-name.css.js // Spectrum styles for element
  - spectrum-component-name.css.js.map // source map
- sp-component-name.d.ts // types
- sp-component-name.js // the side effect full registration of the custom element
- sp-component-name.js.map // source map

In a more expansive (and less theoretical) package like @specrum-web-components/button this would play out in the form of the following directory structure:

- src
  - action-button.css.d.ts // types
  - action-button.css.js // styles for element
  - action-button.css.js.map // source map
  - ActionButton.d.ts // types
  - ActionButton.js // class definition
  - ActionButton.js.map // source map
  - button-base.css.d.ts // types
  - button-base.css.js // styles for element
  - button-base.css.js.map // source map
  - button.css.d.ts // types
  - button.css.js // styles for element
  - button.css.js.map // source map
  - Button.d.ts // types
  - Button.js // class definition
  - Button.js.map // source map
  - ButtonBase.d.ts // types
  - ButtonBase.js // class definition
  - ButtonBase.js.map // source map
  - clear-button.css.d.ts // types
  - clear-button.css.js // styles for element
  - clear-button.css.js.map // source map
  - ClearButton.d.ts // types
  - ClearButton.js // class definition
  - ClearButton.js.map // source map
  - field-button.css.d.ts // types
  - field-button.css.js // styles for element
  - field-button.css.js.map // source map
  - index.d.ts // types
  - index.js // re-exporting of the classes in the package (leverages the `main` and `module` field to prevent from surfacing its location)
  - index.js.map // source map
  - spectrum-action-button.css.d.ts // types
  - spectrum-action-button.css.js // Spectrum styles for element
  - spectrum-action-button.css.js.map // source map
  - spectrum-button-base.css.d.ts // types
  - spectrum-button-base.css.js // Spectrum styles for element
  - spectrum-button-base.css.js.map // source map
  - spectrum-button.css.d.ts // types
  - spectrum-button.css.js // Spectrum styles for element
  - spectrum-button.css.js.map // source map
  - spectrum-clear-button.css.d.ts // types
  - spectrum-clear-button.css.js // Spectrum styles for element
  - spectrum-clear-button.css.js.map // source map
  - spectrum-field-button.css.d.ts // types
  - spectrum-field-button.css.js // Spectrum styles for element
  - spectrum-field-button.css.js.map // source map
- sp-action-button.d.ts // types
- sp-action-button.js // the side effect full registration of the custom element
- sp-action-button.js.map // source map
- sp-button.d.ts // types
- sp-button.js // the side effect full registration of the custom element
- sp-button.js.map // source map
- sp-clear-button.d.ts // types
- sp-clear-button.js // the side effect full registration of the custom element
- sp-clear-button.js.map // source map

This would update the previous example of needing both the side effects and the class listed above to the following:

import { ActionButton} from '@spectrum-web-components/button';
import '@spectrum-web-components/button/sp-action-button.js';

I'm presently cleaning up a branch with this change, and the associated change to the element generator yarn new-package so that this can be more closely review from a technical level and will point it to this issue. As a test case, I've already seen load management benefits from this sort of structure in the westbrook/11ty branch, where the requirements of the application have allowed for the button package to be spread out across multiple bundles thanks to this change.

Assuming this makes sense as we continue down the path towards the maturity of a 1.0, I'd also recommend that we leverage this approach in order to merge the following packages together in a form more congruous with the Spectrum CSS source packages:

  • tab + tab-list => tabs
  • menu + menu-group + menu-item => menu
  • radio + radio-group => radio

Aligning with Spectrum CSS should simplify maintenance long term as we lean more heavily of its source, Spectrum DNA, to support styling/theming across more targets, and reduce confusion when onboarding new users/developers. Merging these things will also help to clarify our documentation/demonstration content that sometimes struggles to find the right way to outline parts of larger things separate from the larger things they need to be a part of to use, see: https://opensource.adobe.com/spectrum-web-components/components/tab and https://opensource.adobe.com/spectrum-web-components/components/tab-list and their nearly identical content.

@adixon-adobe
Copy link
Collaborator

At a high level this all makes sense. I know I'm not going to have time to do a deep dive here, but I'd like @cuberoot to weigh in on this so we have a second set of eyes.

One question I have out of this is whether your example ActionButton import should actually be from @spectrum-web-components/button instead of @spectrum-web-components:

import '@spectrum-web-components/button';
import { ActionButton} from '@spectrum-web-components';

I don't totally grok how the import from @spectrum-web-components here would work.

@adixon-adobe
Copy link
Collaborator

Also, would the use of Scoped Registries be global across SWC packages.e.g. would importing sp-dropdown & sp-menu share the same sp-menu implementation? Or would the menu be scoped separately? (I know this is still a little theoretical, but I'd hope there's just one menu registered by default when this eventually happens)

And then there's the other side of the question -- could scoped registries allow for 2 different versions of SWC to live in the DOM under different names? I could see that being useful for large projects pulling in lots of dependencies. I know this is a real problem with ReactSpectrum for some teams at Adobe.

@Westbrook
Copy link
Contributor Author

Westbrook commented May 21, 2020

Clarification about Scoped Registries

I've not outlined Scoped Registries as a pattern that our components should use, but as a pattern that users of our component may choose to use. While the suggestions above prepare our components to be more flexible to this context, it would not be adopting this context into this library. I don't currently see a benefit to our library doing so, but if we prepare our packages to be used that way, it does also prepare us to use our packages that way if we were to decide that was useful in the future.

One of the main benefits of Scoped Registries is exactly the idea that you could have two different versions of the same <my-element> custom element used in a page. The fact that other web component library authors have begun to run into this enough to create the Scoped Elements tooling mentioned above is a big reason I'd like to start to move in a direction that is compatible with those approaches ASAP, and hopefully, bake the ability to be used as such into our "1.0" release...

@davidyxu
Copy link
Contributor

makes sense to me. just curious how does interact with swc components which depend on other swc-components, will each side-effect import also bring in all of it's dependencies? or does that pattern sort of go away with the reorganizing of some of the packages into singular packages.

@cuberoot
Copy link
Collaborator

This makes sense to me overall. I am also interested in @davidyxu's question about bringing in dependencies.

@Westbrook
Copy link
Contributor Author

Westbrook commented May 22, 2020

The question of where dependencies get loaded is a good one! First, a little context. The following packages are side effect fully depended on internal to the project:

@spectrum-web-components/icon
@spectrum-web-components/underlay
@spectrum-web-components/button
@spectrum-web-components/icons-ui
@spectrum-web-components/menu-item
@spectrum-web-components/popover

A relatively small number of dependencies spread over 14 of the currently 44 packages.

As these are required dependencies of the shadow roots of these packages, it is our packages' responsibility to side effect fully import of these elements. To support that today, I'd like to say we should go with the simplest approach of importing those side effects in the side effect file. In this way, @spectrum-web-components/dropdown/sp-dropdown.ts could include the following:

import '@spectrum-web-components/icon/sp-icon.js';
import '@spectrum-web-components/menu-item/menu-item.js';
import '@spectrum-web-components/popover/sp-popover.js';
import { Dropdown } from './dropdown.js';

/* istanbul ignore else */
if (!customElements.get('sp-dropdown')) {
    customElements.define('sp-dropdown', Dropdown);
}

declare global {
    interface HTMLElementTagNameMap {
        'sp-dropdown': Dropdown;
    }
}

However, I've been reminded of the useful point that then self registrations of our elements would need to know about our elements' internals to ensure everything is registered appropriately, which is not optimal. As such, the child side effects do see to remain in the class imports, which while a pitty does ensure that the default use case is 100% covered on day zero.

import '@spectrum-web-components/icon/sp-icon.js';
import '@spectrum-web-components/menu-item/menu-item.js';
import '@spectrum-web-components/popover/sp-popover.js';

export class Dropdown extend LitElement {
    // etc...
}

There's room from this position for us to be able to push some of these imports into dynamic imports and allow bundlers to make more nuanced bundles for apps leveraging our components internally, which is another growth area that I am interested in this approach opening up to us.

How people are handing child element registrations in scoped contexts beyond that is a good question though. Before actual Scope Registries make it into the browser, there is both a lot of room for shifting in the API and jostling in best practices. No matter where we end up today, I think the first steps outlined herein should shorten the path towards a final implementation based on the conversations I've seen at the spec'ing level. In the interim, I've reached out to some people at ING that wrote/leverage Scoped Elements currently to see what they are doing here. With Scoped Elements managing only a level at a time, it's hard to think how they'd manage without going 100% in on the approach. However, even with a relatively small runtime and filesize cost, I'd lean away from doing that today.

@davidyxu
Copy link
Contributor

hmm if i'm understanding correctly, both the self-registering and the manually-registered versions of a component will self-register their dependencies?

i agree that having to understand the internals to import components is definitely not ideal, but it feels like the self-registering version is probably the route most clients should be going down for convenience anyways. to me it feels like having the manual versions self-registering it's dependencies removes a bit of the value in having the manually-registered versions in the first place.

@Westbrook
Copy link
Contributor Author

Yes, the owner of a dependency will register that dependency as outlined in this sample from above:

import '@spectrum-web-components/icon/sp-icon.js';
import '@spectrum-web-components/menu-item/menu-item.js';
import '@spectrum-web-components/popover/sp-popover.js';

export class Dropdown extend LitElement {
    // etc...
}

There are certainly be short comings to that approach when not fully implementing a scoped approach. This is possibly why some teams (vmware-archive/clarity#4672 (comment)) see scoping as inevitable. In that we've not specifically run into these short comings as yet, I'm not currently driven towards going whole hog on Scoped Elements (the user space predecessor to Scoped Registries), but maybe this is a sign that we should in preparation? While I could be convinced otherwise, that would seem a bit of a step too far right now.

Right now, we do have users leveraging self-registering entry points (as I agree most will for some time) and in doing so with multi-element packages (like @spectrum-web-components/button, @spectrum-web-components/menu-item, @spectrum-web-components/sidenav, et al) are required to download and register multiple elements that might not actually be used in their applications. This ability to tune for performance and feature scope is much more important in the near term, and I'd be happy to leave out conversation around a scoped future in further discussion of the approach suggested above. While they have some alignment, it may simply be too early to say that we can "solve" or even fully "prepare" for what comes of that future.

@bengfarrell
Copy link
Collaborator

bengfarrell commented Jun 12, 2020

I'll add one annoyance in regards to leveraging the index. My linting settings pick up on duplicate modules and warn, and the following is seen as dupe entries:

import { ActionMenu } from '@spectrum-web-components/action-menu';
import '@spectrum-web-components/action-menu';

I can ignore this, but it's so innocuous that my IDE (WebStorm) feels free to remove one when I have other linting issues in the same file and I tell it to auto-fix.

I've had to add in lint ignore comments to stop this from happening because my buttons keep popping out of my component when I'm not watching it like a hawk

@Westbrook
Copy link
Contributor Author

Yikes! We were linting this too, so it would complain all the time. Luckily we weren't using fix and having code stolen from our project like this, however. Updating to the new importing strategy will definitely fix this in a way that should be good for everyone...the people who don't need the classes/types AND those of us that do. 💯

@Westbrook
Copy link
Contributor Author

applied in #683

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

8 participants