Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor plugins and update seed-theme #1

Merged
merged 20 commits into from Jun 21, 2022
Merged

Refactor plugins and update seed-theme #1

merged 20 commits into from Jun 21, 2022

Conversation

ScottPolhemus
Copy link
Contributor

@ScottPolhemus ScottPolhemus commented Jun 15, 2022

The main goal of this PR is to reduce the custom functionality of our plugin to the bare essentials to prepare for an initial "MVP" ("minimum viable product") release. As discussed, this falls into three main features:

  • Automatic entrypoint detection
  • Generate vite-tag liquid snippet for inserting script and link tags for bundled assets into Shopify theme
  • Support for Vite's injected preload link tags and static imported assets which rely on base path

In summary, the plugin should provide a Shopify integration supporting all the basic features provided by Vite.

I made a few changes to how the plugin code is structured, which I'll describe in detail here.

Upgrade Vite to 3.0.0-alpha

Vite 3.0 is in active development and appears to be nearing completion with frequent updates being released to the alpha. I think it's a good idea for us to target Vite 3.0 as the minimum supported version for our plugin. A few reasons for this:

  • This is a new plugin which is unlikely to be dropped into pre-existing Vite installations, so we can safely recommend that users should be using the latest version.
  • By only supporting the latest version, we limit the amount of work we might need to do for backwards-compatibility. (For instance, 3.0 changes the default dev server port from 3000 to 5173. This is configurable but it would be nice not to have to support both defaults.)
  • When Vite 3 is released, the org will certainly be doing some publicity to announce the update and get people excited. With all the additional attention on the framwork, it would be a great time to formally unveil our plugin.
  • Vite 3 includes support for a relative base option, which basically removes the need to implement any dynamic base solution. Setting base: '' or base: './' in version 3.0 allows Vite to generate URLs relative to the current file, which means in our solution things like the preload link tags will now have the correct URLs by default.

Refactor plugin config/options

Based on what I've seen in documentation and in other Vite plugin code, "config" usually refers to the settings passed directly into Vite (e.g. through vite.config.js) while "options" can refer to the options passed in to the plugin constructor.

I split out the logic for generating the config for Vite into a separate sub-plugin, and created options.ts to define the types for options supported by the plugin itself. I also implemented a pattern where the options presented by the user are "resolved" using a function in options.ts that resolves them by filling in defaults as needed, similarly to how Vite handles its own config settings.

Refactor HTML plugin

I found the code in the HTML plugin to be difficult to follow when reading through, and overly complex. Part of the reason for this is the logic needed to parse through the bundle and determine the correct entrypoints and assets, which used different logic for the dev server and production build.

My alternate solution is different in a few ways:

  • In development, the snippet does not need to know about the bundle chunks. We don't need to render tags for imported CSS because it is not extracted in development, and preload tags are not used either. Therefore we can just pass through the given entry point to the script tag directly, simplifying the logic needed in the liquid snippet itself. For instance:
# Liquid:
{{ render 'vite-tag' with 'theme.js' }}
# HTML Output:
<script src="http://localhost:5173/frontend/entrypoints/theme.js"></script>

# Liquid:
{{ render 'vite-tag' with 'styles/main.css' }}
# HTML Output:
<link href="http://localhost:5173/frontend/styles/main.css" rel="stylesheet" />
  • In production, the recommended way to implement back-end integrations is to use the manifest.json file. As you might have noticed, this file does not include information about any CSS entry points. This is a major drawback which is currently being addressed in this Vite PR. In the meantime, there is a workaround which involves copying Vite's built-in manifest plugin and applying it as a user plugin instead. I decided to drop in this solution and refactor the HTML plugin to reduce the amount of custom logic we need to implement to parse through the bundle chunks. I also rewrote the functions for generating tags to use template strings to make the code more readable.

Add support for CSS image asset URLs

The built-in relative base option doesn't support images which are imported into CSS through url().

When you use url() to reference an image in the theme code:

  • In development, Vite processes the CSS so that it references the image path with a leading / to make it relative to the current domain.
  • In production, Vite copies the image to the output folder and replaces the CSS url with a reference to the new file, again using a leading / to load it relative to the domain.

The plugin that I added makes the following adjustments to allow this mechanism to work better with Shopify:

  • In development, we prepend the image URL with the Vite dev server host, so that it will load when accessing the site from a different domain (e.g. the Shopify CLI server or preview links).
  • In production, we strip the leading / from the URL so that it loads as a relative path, meaning it will automatically pull from the same CDN folder as the CSS asset itself.

Bonus Updates: seed-theme and vite-plugin-shopify-modules

In addition to the work on the core plugin, there are some additional updates:

  • Implemented vite-plugin-shopify-modules to create symlinks for module liquid files
  • Added my latest development code for the base theme into seed-theme

@@ -0,0 +1,102 @@
// NOTE: Once https://github.com/vitejs/vite/pull/6649 has been released, we can remove this and use the built-in manifest option.
Copy link
Contributor

Choose a reason for hiding this comment

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

@ScottPolhemus vitejs/vite#6649 has been merged 🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the current implementation in the 3.0.0 beta is not sufficient for this use-case. A couple issues I noticed:

  • CSS entry points do not get isEntry: true in the manifest file.
  • The source file extension is shown as .css for all files, even if they're using a different file entry format like .scss.
    For those reasons, I left the workaround in place for now. We'll see if they get resolved before the final 3.0.0 release.

assign file_extension = vite-tag | split: '.' | last
assign css_extensions = '${KNOWN_CSS_EXTENSIONS.join('|')}' | split: '|'
assign is_css = false
for css_ext in css_extensions
Copy link
Contributor

Choose a reason for hiding this comment

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

@ScottPolhemus how about using the contains operator?

+ if css_extensions contains file_extension
+ endif
- for css_ext in css_extensions
- endfor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know about that, thank you! Updated.

@@ -13,5 +13,5 @@ html {

body {
min-height: 100%;
background-image: url('@/images/cool-background.png');
background-image: url('@/frontend/images/cool-background.png');
Copy link
Contributor

Choose a reason for hiding this comment

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

@ScottPolhemus ~ and @ are aliases for the frontend folder, we can exclude it from the path

- background-image: url('@/frontend/images/cool-background.png');
+ background-image: url('@/images/cool-background.png');

Copy link
Contributor

Choose a reason for hiding this comment

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

@ScottPolhemus Currently we are getting 404 from image assets, Could we check the previous implementation?

expected behavior: https://imgur.com/a/N2IdmoZ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I went back and updated the plugin to fix the broken development URLs when using an alias-ed import path.

name: 'vite-plugin-shopify-config',
config () {
const generatedConfig: UserConfig = {
// Use relative base path so imported assets load from Shopify CDN
Copy link
Contributor

Choose a reason for hiding this comment

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

The linter is not passing, looks like we got some spacing issues in this line

- // Use relative base path so imported assets load from Shopify CDN
+ // Use relative base path so imported assets load from Shopify CDN
 

import shopifyModules from 'vite-plugin-shopify-modules'

export default defineConfig({
mode: process.env.NODE_ENV === 'production' ? 'production' : 'development',
Copy link
Contributor

Choose a reason for hiding this comment

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

Vite handles this setting for us automatically, we should be able to avoid setting this up manually

`{{ '${fileName}' | asset_url | stylesheet_tag }}`

// Generate vite-tag snippet for development
const viteTagSnippetDev = (assetHost = 'http://localhost:5173', entrypointsDir = 'frontend/assets'): string =>
Copy link
Contributor

Choose a reason for hiding this comment

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

@ScottPolhemus In our previous implementation we had the root of vite set to the entrypoints directory for cleaner entry urls

http://localhost:3000/theme.ts

http://localhost:3000/pages/pdp.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, IMO that approach assumes too much about the file structure of the theme code. (Leaving the root alone allows users to import files that are not nested inside the frontend directory if they want.)

@montalvomiguelo
Copy link
Contributor

montalvomiguelo commented Jun 21, 2022

This is great @ScottPolhemus 🥇 I just added a couple small comments.

What if we integrated this into main so we can continue development.

Maybe we could add a note to our readme like "The main branch is now for vite v3.0, if you are looking for current stable releases, check the v0 branch instead"

Scott Polhemus added 5 commits June 21, 2022 14:41
- Bumped Vite to 3.0.0 beta release
- Added changeset for generating changelogs and bumping package versions
- Updated readme documentation
@ScottPolhemus
Copy link
Contributor Author

Thanks @montalvomiguelo, made some updates and merged to the main branch!

@ScottPolhemus ScottPolhemus deleted the scott/develop branch June 21, 2022 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants