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

Add AssetSizeJavascript Check #194

Merged
merged 15 commits into from
Mar 11, 2021
Merged

Conversation

charlespwd
Copy link
Contributor

@charlespwd charlespwd commented Mar 9, 2021

Related #189 (part 1: JS)

Add an AssetSizeJavaScript check + docs. The goal being to discourage
the use of frameworks and encourage web native.

Excerpt from the checks' docs (general template adapted from ESLint):

Prevent JavaScript Abuse on Server Rendered Themes (AssetSizeJavaScript)

For server rendered pages, it is an anti-pattern to execute large JavaScript bundles on every navigation.

This doesn't mean they don't have a reason to exist. For instance, chat widgets are mini applications embedded inside web pages. Designing such an app with server rendered updates would be absurd. However, if only 10% of the users interact with the chat widget, the other 90% should not have to execute the entire bundle on every page load.

The natural solution to this problem is to implement the chat widget using the Import on Interaction Pattern.

Rule Details

This rule disallows the use of theme JavaScript files and external scripts to have a compressed size greater than a configured threshold_in_bytes.

👎 Example of incorrect code for this rule:

<!-- Here assets/chat-widget.js is more than 10KB gzipped. -->
<script src="{{ 'chat-widget.js' | asset_url }}" defer></script>

<!-- The use of jQuery is discouraged in themes -->
<script src="https://code.jquery.com/jquery-3.6.0.min.js" defer></script>

👍 Example of correct code for this rule:

<script>
  const chatWidgetButton = document.getElementById('#chat-widget')

  chatWidgetButton.addEventListener('click', e => {
    e.preventDefault();
    import("{{ 'chat-widget.js' | asset_url }}")
      .then(module => module.default)
      .then(ChatWidget => ChatWidget.init())
      .catch(err => {
        console.error(err);
      });
  });
</script>

See docs/checks/asset_size_javascript.md for more details.

@charlespwd charlespwd force-pushed the feature/js-bundle-size-check branch from 15626a8 to c538e04 Compare March 9, 2021 21:35
Copy link
Contributor

@macournoyer macournoyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some early comments/questions

Copy link
Contributor

@macournoyer macournoyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Straight fire 🔥


module ThemeCheck
module RegexHelpers
def matches(s, re)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC what this code does, you're looking for String#scan: https://ruby-doc.org/core-3.0.0/String.html#method-i-scan

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really want the MatchData since I'm using match.begin(:src) to get the index of the src inside the string. And scan gives me strings IIRC.

Copy link
Contributor

@macournoyer macournoyer Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use https://ruby-doc.org/core-3.0.0/Regexp.html#method-c-last_match. But you have to use scan w/ a block.

@charlespwd charlespwd merged commit a122646 into master Mar 11, 2021
@charlespwd charlespwd temporarily deployed to rubygems March 12, 2021 21:00 Inactive
@charlespwd charlespwd deleted the feature/js-bundle-size-check branch March 16, 2021 19:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants