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

feat: build constructed stylesheet along side css file #68

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

digitalsadhu
Copy link
Collaborator

Summary

This PR adds a build for a constructed stylesheet via Lit's css template literal function.

Motivation

We are currently trying to bypass css scoping limitations for the purposes of writing Shadow DOM enabled apps and components. We want to be able to efficiently share styles across Shadow DOM boundaries without incurring a perf penalty. Our early experiments suggest that constructed stylesheets may way help us with this.

What does this PR do?

This refactors the build slightly so that we produce both a css and js version of the Fabric styles. The Eik version of the js version has to be handled slightly differently due to the use of a bare import on lit.

Background reading

@digitalsadhu digitalsadhu requested review from trygve-lie and a team March 15, 2022 15:22
@pearofducks
Copy link
Contributor

lgtm! i assume @import url('@fabric-ds/css'); that we're using for Eikification in css doesn't care about the addition of an exports field?

@digitalsadhu
Copy link
Collaborator Author

digitalsadhu commented Mar 16, 2022

lgtm! i assume @import url('@fabric-ds/css'); that we're using for Eikification in css doesn't care about the addition of an exports field?

Should be fine but will double check

EDIT: It shouldn't make a difference as we just map the module name to a url:

See: https://assets.finn.no/map/fabric/2.0.0

{
  "imports": {
    "@fabric-ds/css": "https://assets.finn.no/pkg/@fabric-ds/css/v1/fabric.min.css"
  }
}

And this PR still publishes fabric.min.css to the same place

@digitalsadhu
Copy link
Collaborator Author

Notes from discussions

Lit's css template literal function returns a CSSResult object which is a Lit thing but contains a property stylesSheet which is a CSSStyleSheet object. So we can just export that property and we will be producing a proper CSSStyleSheet object. We could also do it manually. In either case, I tested and Lit supports both CSSResult and CSSStyleSheet objects when you define a components styles which is super nice. Eg. Both of these work in Lit!

static styles = [
    styles.styleSheet,
    css``
  ];

and

static styles = [
    styles,
    css``
  ];

So, easy enough to export a CSSStyleSheet natively...

#!/usr/bin/env node

import fs from 'fs';

const input = new URL('../dist/npm/fabric.min.css', import.meta.url);
const output = new URL('../dist/npm/fabric.min.js', import.meta.url);

const styles = fs.readFileSync(input.pathname, 'utf8');
fs.writeFileSync(output.pathname, `const sheet = new CSSStyleSheet();sheet.replaceSync(\`${styles}\`);export default sheet;`);

but since this is quote poorly supported (no safari and behind a flag in firefox) we are probably going to need to use Lit here.

@trygve-lie
Copy link
Collaborator

I think we should keep this as highly experimental and be prepared to revert this if we merge it. Maybe we should look a lot more into CSS Modules first. With early tests I've done it seems to me that CSS Modules works very well out of the box with Lit.

Then its just a matter of doing so in the Web Components [1]:

import styles from './styles.css' assert { type: 'css' };

And then the document can load the same styles as it already do.

We might have to take an extra peak on the Eik build plugins to support the import assertion syntax. Specially the web pack plugin where we had to do the import parsing on our own.

1 - A tiny polyfill for some browsers is needed.

@digitalsadhu
Copy link
Collaborator Author

digitalsadhu commented Mar 17, 2022

Yea, from reading as I understand it, that would avoid the need to explicitly build the constructible stylesheet ourselves as the import/asset seems to wrap the styles in a constructible stylesheet on the fly which is super nice. Not easy to polyfill at runtime but should be ok at build time I'm guessing. Might be we can just handle it all in the Fabric elements library for now?

EDIT: I can't even find a way to transpile this at build time to provide backwards compat. Wondering if it might be a bit early to start using?

Lit recommends we use their layer to handle constructible stylesheets: https://lit.dev/docs/components/styles/?linkId=8044902#sharing-styles

@benja
Copy link
Contributor

benja commented Mar 17, 2022

Just had the chance to read through this PR and a bit up on constructable stylesheets as well as the assert on import method. It seems to me as the assert method is a bit premature (?) and we should hold up a bit on that. I would say as we're moving forward with Lit to stick to their recommendations as much as possible, but be open to opportunities such as defining our own CSSStyleSheet and passing this to Lit's styles method, rather than using lit's built in css function for example. But that's all up for debate, and we can always experiment down the line with this at a later point.

I agree with @trygve-lie's point on making it clear to people that this project is experimental, which gives us the freedom to change direction down the line if we see the benefit in doing so. Whatever we choose to go with should be fine, as I think we are the first ones to mess around and build stuff with this either way, leaving us with even more freedom imo.

@digitalsadhu
Copy link
Collaborator Author

We might consider dropping this experiment entirely in favour of CSS import asserts. Something like the following will work in Chrome to import Fabric directly from Eik into a constructed stylesheet.

import styles from 'https://assets.finn.no/pkg/@fabric-ds/css' assert { type: 'css' }

To support other browsers, we can use https://github.com/guybedford/es-module-shims

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

Successfully merging this pull request may close these issues.

4 participants