Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

feat(styles-loader): aggregate stylesheets and dedupe if already loaded #1291

Merged
merged 3 commits into from
Feb 19, 2024

Conversation

zacowan
Copy link
Member

@zacowan zacowan commented Feb 7, 2024

Description

This change ports #1099 from one-app@6 to one-app@5.

Motivation and Context

This change is raised to support an existing one-app@6 feature in one-app@5. For the original motivation and context, please see the original PR (#1099).

How Has This Been Tested?

To test this change, I generated a root module and two child modules which all render on a single page. Within each module, I have installed a basic component library that imports a css module within the component:

// Button.jsx
import styles from './styles.module.css';
// ...
<button {...otherProps} className={clsx(styles.button, className)}>
  {children}
</button>
/* styles.module.css */
.button {
  background-color: blue;
  color: white;
  border: none;
  padding: 8px 16px;
}

Within each module, I've also created a styles.module.css with a class that changes the background color of the element it is applied to. This class has been applied to an instance of the Button component from the component library listed above with the text "With Changes".

Note: the following pictures list the installed versions of one-app-bundler, but the relevant change is within the dev bundler. The installed version of the dev bundler (1.6.0 in root and child-a, 1.5.5 in child-b) in all cases includes the relevant change needed to support this feature.

Before this change, the response from the server would look like this (note the duplicated button class and the lack of style overrides on the root and child-a):

before-visual
before-head

After this change, the response from the server looks like this (note the single button class and the style overrides across all modules):

after-visual
after-head

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (adding or updating documentation)
  • Dependency update
  • Security update

Checklist:

  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • These changes should be applied to a maintenance branch.
  • This change requires cross browser checks.
  • Performance tests should be ran against the server prior to merging.
  • This change impacts caching for client browsers.
  • This change impacts HTTP headers.
  • This change adds additional environment variable requirements for One App users.
  • I have added the Apache 2.0 license header to any new files created.

What is the Impact to Developers Using One App?

An existing feature on one-app@6 will be available to users still using one-app@5.

@CLAassistant
Copy link

CLAassistant commented Feb 7, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

github-actions bot commented Feb 7, 2024

Size Change: 0 B

Total Size: 694 kB

ℹ️ View Unchanged
Filename Size
./build/app/app.js 159 kB
./build/app/app~vendors.js 400 kB
./build/app/runtime.js 7.07 kB
./build/app/service-worker-client.js 7.25 kB
./build/app/vendors.js 120 kB

compressed-size-action

@zacowan zacowan marked this pull request as ready for review February 7, 2024 18:17
@zacowan zacowan requested review from a team as code owners February 7, 2024 18:17
@JAdshead JAdshead enabled auto-merge (squash) February 19, 2024 16:23
@JAdshead JAdshead merged commit e840a87 into 5.x.x Feb 19, 2024
8 checks passed
@JAdshead JAdshead deleted the feat/aggregate-ssr-styles-loader-v5 branch February 19, 2024 16:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants