1.3.1 (patch) release included breaking changes (auto-injecting css)

[NiklasPor]

Describe the bug 📝

Hey all, the 1.3.1 patch version (marked as latest) included a breaking change, which should not be the case, as it's just minor not a major release.

This is the release commit, which is title 1.3.0 but the npm package version is 1.3.1: https://github.com/IFCjs/components/commit/5bff295d3aecedcdcdac76b651c0cc1f3bd91d37

The auto-inject of styles broke our app, by simply applying global styles automatically (we had manual stylings which were scoped before). For example all scroll bars in our app now look purple.

This is the PR which added the auto-injection of styles (which is a good idea, although it should be toggleable): https://github.com/IFCjs/components/pull/218

But this should not happen during a patch release, as those are expected to be only bugfixes and not any major changes which requires migration by the consumers of the packages.

I'd vote to revert those changes and wait for the next major release. Until the major release we can make the styles injectable by calling a method explicitly (like injectStyles). Then, with the next major release, inject them as a default and make it possible to disable that behaviour.

Reproduction ▶️

No response

Steps to reproduce 🔢

No response

System Info 💻

System:
    OS: macOS 14.1
    CPU: (14) arm64 Apple M3 Max
    Memory: 92.34 MB / 36.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.19.0 - ~/Library/pnpm/node
    npm: 10.2.3 - ~/Library/pnpm/npm
    pnpm: 8.12.0 - ~/Library/pnpm/pnpm
  Browsers:
    Chrome: 120.0.6099.234
    Safari: 17.1
  npmPackages:
    openbim-components: 1.3.1 => 1.3.1 

### Used Package Manager 📦

pnpm

### Error Trace/Logs 📃

_No response_

### Validations ✅

- [X] Read the [docs](https://docs.thatopen.com/intro).
- [X] Check that there isn't [already an issue](https://github.com/IFCjs/components/issues) that reports the same bug to avoid creating a duplicate.
- [X] Make sure this is a repository issue and not a framework-specific issue. For example, if it's a THREE.js related bug, it should likely be reported to [mrdoob/threejs](https://github.com/mrdoob/three.js) instead.
- [X] Check that this is a concrete bug. For Q&A join our [Community](https://people.thatopen.com/).
- [X] The provided reproduction is a [minimal reproducible example](https://stackoverflow.com/help/minimal-reproducible-example) of the bug.

[janruo]

Not only is the library casually polluting global :root, *, ::before, ::after, html, .container, scrollbars etc, but the way the stylesheet is loaded is also pretty crazy:

    const fetchResponse = await fetch(
      "https://raw.githubusercontent.com/IFCjs/components/main/resources/styles.css"
    );

So resources/styles.css can never be renamed or deleted from the repository, or existing installations will suddenly just lose their styles. Also good luck trying to even maintain that file when all changes to it will also affect previous versions 👍

[HoyosJuan]

Hi @NiklasPor!

Are you using the built-in UI from components?

[NiklasPor]

Actually not, but some components throw errors if the UI isn't enabled, so I still have it enabled (but I'm not rendering anything with components). The issue is rather about the breaking-change in a minor version (because global styles are automatically imported).

[HoyosJuan]

Ok, so a couple of things here:

1. You're right, we shouldn't release this as a patch. The reason why it was like that, is because we made a mistake in which we release 1.3.0 to NPM without merging the changes for that minor version, so the only solution was to merge all changes and publish them as a patch to the current version. Let's hope this doesn't happen again.

2. About the breaking changes, we don't consider this to be a breaking change as an app made with 1.2.x which upgrades to 1.3.x doesn't need to change the codebase drastically, in fact everything should be working the same. However, as all releases, there is always room for bugs that lead to patches in the current version. That last thing leads me to the next point.

3. components.uiEnabled = false not only removes the UI, but it also doesn't run the code to include the CSS. Now, as you mentioned, some components may throw errors when the UI is not initialized (which forces you to have them enabled). Would you please give me a list of components that throws you errors if the UI is disabled? That way I can fix them, publish a new patch, and then you can safely disable the UI components so it doesn't include the CSS. In the meantime, the auto included CSS is a style tag set in the header with "openbim-components" as its ID. After you initialize the viewer, you can remove that style tag without any problem if you're not using the built in UI Components.

4. About @janruo, you're right... the CSS may be polluting some global styles. That doesn't have anything to do with the new release, it was like that previously. Of course, that doesn't mean is right, we need to fix it maybe by scoping them to the viewer container as all the other styles in the file.

5. You're also right @janruo about linking to the same styles in GitHub, as it can cause problems with future version. We've already spot that problem, and probably will be linking the styles the same as we link the wasm files.

[NiklasPor]

Awesome, all of this sounds good. I'd obviously disagree on the breaking change part, as the update broke the styles of our app.

For the uiEnabled issue, acrtually I don't have it anymore. Might've been in an older version (I didn't check if this still appears every upgrade).

Thanks for all the hard work 👏

[NiklasPor]

@HoyosJuan actually I still get one error when I disable the ui, the components.dispose() call throws:

(editor.ts is my own code, where I call components.dispose())

Should I open a separate issue?

[HoyosJuan]

I don't think is needed, I will take a look at it!

[cytostatic]

I have another suggestion to the auto injected styles. It would be nice to have the compiled styles and also the "Material Icons" font in the npm package to load it manually. Loading google fonts from their website is kind of critical especially in the EU because of GDPR.

[myabeaver]

In our case, we need the UI component, but would like to make some style adjustments, e.g. no scrollbar styles. The best would be if you load the CSS file from the node_modules, without URL dependencies.

And there should be a way to disable loading in the UI manager.

Update: I've created a PR https://github.com/IFCjs/components/pull/289 . Named it globalStylesEnabled. The idea would be to then be able to disable it with:

components.globalStylesEnabled = false;

[agviegas]

The auto-injected CSS issue should be solved in the latest version of the libraries, as we have decoupled all the UI-related logic to a separate library. As for the breaking changes - sorry. We are a small team advancing fast, but from now on, all patch versions should be non-breaking. If that's not the case in the future, let us know! Cheers