-
Notifications
You must be signed in to change notification settings - Fork 3
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
base: main
Are you sure you want to change the base?
Conversation
lgtm! i assume |
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 |
Notes from discussionsLit's css template literal function returns a static styles = [
styles.styleSheet,
css``
]; and
So, easy enough to export a #!/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. |
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. |
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 |
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. |
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.
To support other browsers, we can use https://github.com/guybedford/es-module-shims |
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