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

feat(devBundler): aggregate styles into dependencies and local overrides #557

Merged
merged 9 commits into from
Sep 5, 2023

Conversation

eddhurst
Copy link
Member

@eddhurst eddhurst commented Aug 30, 2023

Provide a general summary of your changes in the Title above.

Description

Describe your changes in detail

This PR achieves two primary features:

  • It aggregates S/CSS files to allow for dependencies to be loaded first, with local application overrides applying last. When rendered into the DOM, this means that application styles that override component library styles don't need to fight for specificity because they will naturally appear last in the cascade.
  • It adjusts the file hash to use the file contents, rather than the file absolute path. This allows files to be deduplicated if they are being rendered into the page, particularly for styles loaded from dependencies such as component libraries, because the file contents will be the same between modules, even when the absolute path is not.

Coupled with an adjustment to the One App style loader, this will allow styles to be rendered into the page consistently between Client and Server renders, remove an unnecessary style recalculation step from the browser, and remove duplicated styles being rendered onto the page for each module.

Motivation

Why is this change required? What issue does it resolve?

Currently when bundling a One App module, the styles are added to the page inconsistently between the server and client side bundles. The server bundle adds the styles "upside down" with local application styles appearing first, and dependency styles appearing last.

In addition, once the client bundle has hydrated the styles to the expected order, duplicate style tags are rendered multiple times, resulting in local application styles being ignored.

Test Conditions

Describe in detail how the changes are tested.

Include details of your testing environment, and the tests you ran to.

How does your change affect the rest of the code.

Tested running a root module with three child modules each serving a shared dependency component library. Each module renders a component twice. Once with no changes, and once with style overrides local to the module.

The expected output is three modules, showing a green component, and then a blue/red component with text that has an orange background color.

The expected DOM output was three style tags, one with the styles for the shared component (deduplicated twice), one with the styles for the blue override (deduplicated once between Module One and Two), and one with the styles for the red override.

Screenshot 2023-08-29 at 16 10 37 Screenshot 2023-08-29 at 22 30 17

For backwards compatibility, a getFullSheet function is included that returns a single string of all the aggregated styles as a string. This allows for running an updated bundle in a version of OneApp that doesn't support the style aggregation.

To note - this getFullSheet does correctly organise the styles to put dependencies before local overrides, which would partially solve the problem where styles are inconsistent between SSR and CSR, so even in an older version of One App, a modern bundle would result in more consistency when hydrated.

Types of changes

Check boxes that apply:

  • 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

Check boxes that apply:

  • My code follows the code style of this project.
  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • These changes should be applied to a maintenance branch.
  • I have added the Apache 2.0 license header to any new files created.

@Matthew-Mallimo Matthew-Mallimo requested a review from a team August 30, 2023 15:41
@JAdshead
Copy link
Contributor

It adjusts the file hash to use the file contents, rather than the file absolute path. This allows files to be deduplicated if they are being rendered into the page, particularly for styles loaded from dependencies such as component libraries, because the file contents will be the same between modules, even when the absolute path is not.

This feature needs to have parity across development and production environments, as this bundler should only be used in development deduplication needs to also be possible using one-app-bundler.

@eddhurst
Copy link
Member Author

This feature needs to have parity across development and production environments, as this bundler should only be used in development deduplication needs to also be possible using one-app-bundler.

100% I wanted to raise this as an initial proof of concept that everybody was happy with the structure and approach before replicating the work into the prod bundler

Would we rather that it were 2 PRs to different packages, or 1 PR for the feature?

@JAdshead
Copy link
Contributor

JAdshead commented Aug 30, 2023

This feature needs to have parity across development and production environments, as this bundler should only be used in development deduplication needs to also be possible using one-app-bundler.

100% I wanted to raise this as an initial proof of concept that everybody was happy with the structure and approach before replicating the work into the prod bundler

Would we rather that it were 2 PRs to different packages, or 1 PR for the feature?

It's a cool feature, up to you on how you do it. I think as long as its noted that PR is open(could be a draft) to the other bundler i don't think it needs to block this PR

@Matthew-Mallimo Matthew-Mallimo merged commit 0eeee49 into main Sep 5, 2023
5 checks passed
@Matthew-Mallimo Matthew-Mallimo deleted the feature/style-aggregate branch September 5, 2023 15:49
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.

5 participants