Consider a key for moving styles into web_modules

[calebdwilliams]

While this isn't necessarily a standard use case for either Pika or npm, would you consider an option to move CSS files over to web_modules/? While Pika currently uses the module key in a dependency's package.json, it could be useful to consume some CSS files or other assets (icons, images, etc) from a dependency (think design systems). I don't have a real proposal for what that key should be, but this could be an incredibly useful feature especially for enterprise.

[FredKSchott]

I'd love to have an example of a package with a decent number of users that uses CSS (Bulma or Bootstrap maybe?).

I think Rollup has a CSS plugin that would move it into web_modules/ as a common asset automatically, which would be perfect. If anyone wants to take the lead on investigating this, I'd be happy to prioritize reviews/responses.

[calebdwilliams]

Looks like both Bulma and Bootstrap just have a style key in their package.json. I'd be happy to take a stab at this.

[FredKSchott]

That would be great! From a quick search it looks like this would be a good starting point: https://github.com/egoist/rollup-plugin-postcss

Some other random thoughts:

• It seems like most CSS-related packages tell you to import the CSS file directly (ex: import 'bulma/css/bulma.css'), so supporting {"webDependencies": ["bulma/css/bulma.css"]} would be a fine place to start.
• As a future enhancement, if a package only has a "style" key and no "main" / "module" / "browser" and no default top-level "index.js" then it would be possible to add support for {"webDependencies": ["bulma"]} as well. Actually, in that case maybe we just support both "style" & "module" by default if both exist...
• In the codebase everything assumes that files are JS, so just watch out for that, especially around file extensions.
• To test, I would just recommend creating a test directory that contains the package.json you'd like to support and then running your package locally after building it. Something like: ../PIKA_WEB_CLONE_DIR/pkg/dist-node/index.bin.js --clean

[calebdwilliams]

Perfect. I'll get this in my backlog. One random thought I'd like to verify before getting started: Pika should only support compiled CSS, correct? It would be possible to support SCSS, LESS, Stylus et al should be pre-compiled. I know rollup-plugin-postcss can handle each of those variants, but it doesn't feel like a good strategy (to me at least), but I'm happy to defer that decision.

[FredKSchott]

Yea, I think you're on to something there. My approach to these sorts of things is "lets add the thing that we wanted, and then see who asks for the extra thing". So let's add CSS support only to start, and then see if anyone even wants SCSS/LESS/Stylus support and then we can have that convo.

Thanks for offering to tackle!

[calebdwilliams]

OK, so I have this feature added, but am not entirely sure how I should proceed if two packages use the same filename (index.css or similar). Any preferences right now?

For the record, it's a pretty naïve implementation right now: I just look for the file an copy it over however it is presented (without manipulation) using fs.copyfile. Any qualms with that approach? I initially tried generating a JS file and using Rollup to and rollup-plugin-postcss, but that seemed like more effort than it was worth.

[FredKSchott]

Yea, let's start with that, easy and simple. That also lets us kick the "I want to import a CSS file inside of JS" discussion down the road while the different native CSS import proposals/discussions play out:

• https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/master/CSSModules/v1Explainer.md
• Cascading Style Sheet module scripts WICG/webcomponents#759

[calebdwilliams]

Love it. Any ideas on potential naming conflicts? Once I get that hammered down (and my company's open source office approval), I'll submit a PR.

[FredKSchott]

I was imagining a straight copy from node_modules -> web_modules. So;

• webDependencies: ["bulma/css/bulma.css"] -> web_modules/bulma/css/bulma.css
• If you want to add "style" entrypoint resolution: - webDependencies: ["bulma"] -> web_modules/bulma.css.

But maybe i'm misunderstanding what you mean by naming conflicts. If so, can you give me an example of what you mean?

[calebdwilliams]

The idea was to just port over whatever file is referenced in the "style" entry in the package (assuming it is CSS and not some other file type). So for Tachyons, it would bring over css/tachyons.min.css into web_modules/ (so web_modules.tachyons.min.css).

Ideally there would be a common directory structure where we could bring over the entirety of a library's output, but there doesn't seem to be a real standard there. Even Bulma, funny enough, doesn't point its style property at an actual file because it adds the /bulma prefix to it (although I've tested what I've written with Bootstrap, Bulma, Shed-CSS, Tachyons and Tailwind and Bulma is the only one to fail so far).

[FredKSchott]

Gotcha, how do you feel about using the strategy above instead ({webDependencies: ["bulma/css/bulma.css"]} -> web_modules/bulma/css/bulma.css)? That way we can follow the same strategy that we're already using to reference files within packages.

Preserving the existing folder structure also allows you to include multiple CSS files if any @imports exist.

[calebdwilliams]

Would that only work as opt-in then? Or would the CLI still work?

EDIT
I think I see what you're saying now. Would you assume, then, that if the @pika/web field is missing from the consuming package.json that no styles would be moved over?

[FredKSchott]

yea, exactly. Right now only ESM packages (those that expose a "module") are included by default. At least to start, this can be an opt-in feature through webDependencies config only (I would imagine some people getting surprised to see CSS in their web_modules/ directory without opting in).

We can release it that way to start, get some early feedback, and then consider adding features / making it default if there's interest.

[calebdwilliams]

Perfect. I'll get that up and running. Thanks for talking me through it.

[calebdwilliams]

So right now this feature will only work if isExplicit === true, do you think there would be any value in adding a flag for the isExplicit === false or just run with it for the time being? Other than that question, I have this ready to go once I get approval.

[calebdwilliams]

Sorry for the barrage of comments here, but I've actually been giving this some thought while using this feature locally and I want to propose a change to the scope of this and would love your feedback.

As an employee at a large enterprise company with our own internal npm, design system including style assets, icons, etc. I believe it would be beneficial to introduce a separate "assets" key to the @pika/web property in the package.json that would copy over a series of white-listed assets including CSS, fonts and images (SVG particularly).

{
  "name": "@corporatescope/some-package",
  "@pika/web": {
    "webDependencies": [
      "lit-element"
    ],
    "assets": [
      "@corporatescope/design-system/system.min.css",
      "@corporatescope/design-system-icons/directory?"
      "@corporatescope/design-system/fonts/custom-font.woff"
    ]
  }
}

Importing assets would always need to be explicit, but wouldn't otherwise affect the web dependencies/Rollup of ES modules.

[FredKSchott]

no problem at all! Thanks for being so thoughtful with this change, it's very much appreciated!

So if we're going down the asset road, it sounds like we have two options:

1. The 2-value config outlined above

2. A single-value config, that's more powerful but potentially more magic

// This config:
{
  "@pika/web": {
    "include": [
      "lit-element",
      "@corporatescope/design-system" // Future work: include all CSS, Fonts, etc. for a package.
    ]
  }
}

// Leads to this web_modules/:
- lit-element.js
- @corporatescope/design-system.js (if "module" or "main" exists)
- @corporatescope/design-system/* (all {css,svg,woff,etc} assets, if any exists)

Tbh I like that option 2. doesn't get more complicated as we add features/support. I don't think it would be too magic, given that most packages don't contain CSS or Fonts and that we would still support current "file path instead of package name" fallback. I guess the one thing to make sure we handle there are asset packages that have no JS in them (no "module" or "main" entrypoint, then maybe we check for a "style" entrypoint and accept that?).

Either way, for now for the scope of this first PR it makes sense to just start by adding support for non-JS file paths in "@pika/web"."webDependencies". This would get us support for CSS, Fonts, SVGs and any other non-JS assets without needing to force a larger interface change/addition. It would also be good to try out the current interface first before adding a new config, to get a better sense of whether we need it.

Another good idea that's already been requested would be glob support, so that you could do something like "@corporatescope/design-system-icons/**/*{css,svg}". If you're interested in tackling this, feel free to do so in a follow up PR / not tackle immediately.

Does that make sense?

[calebdwilliams]

Yeah, that makes sense. What I have already will allow for some future options with small modifications, but I'll just leave this initial PR as explicitly-added non-JS files.

Thanks.

[FredKSchott]

perfect, can't wait to see!

[qubyte]

Lending weight to this, blink announced intent to implement v1 CSS modules: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/vCrJCQxNnzo/OAYI9cPWAQAJ

The tl;dr:

import sheet from "./styles.css";

document.adoptedStyleSheets = [...document.adoptedStyleSheets, sheet];

[FredKSchott]

Nice! I think that's a reason to keep CSS in the web_modules/ directory, if we'll be moving towards importing CSS in JS on the web.

Closed via #85