Pro 6694 breakpoint preview vite ready #9

ValJed · 2024-11-04T15:57:45Z

PRO-6694

Summary

Setups new plugin for mobile preview in the vite base config.

What are the specific steps to test this change?

Should be tested with this plugin when released.

⚠️ It's the reason tests are red.
Branch target should be removed before to merge.

What kind of change does this PR introduce?

Make sure the PR fulfills these requirements:

It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
The changelog is updated
Related documentation has been updated
Related tests have been updated

linear · 2024-11-04T15:57:49Z

PRO-6694 Breakpoint preview CSS loader logic should be Vite-ready

ValJed · 2024-11-05T16:05:36Z

lib/vite-postcss-config.js

+  const {
+    plugins = [],
+    ...postCssConfig
+  } = importedConfig.default || importedConfig;


Supports cmj and esm.

We need the official postcss loader.

ValJed · 2024-11-05T16:06:53Z

lib/vite-postcss-config.js

+  const postCssConfigPath = path.resolve(process.cwd(), 'postcss.config.js');
+  const importedConfig = isPublic && fs.existsSync(postCssConfigPath)
+    ? await import(postCssConfigPath)
+    : {};


Only try to get postcss project config file if in public mode.
We might use one in apostrophe for other plugins like autoprefixer?

Autoprefixer for Apos might make sense. I don't think Vite does anything related, I can't recall they are doing it by default (probably inspecting the relevant docs/research a bit is a good idea). If we decide to do that, you can add it directly in the apos base config (vite-apos-config.js). It will be merged with anything that potentially comes later.

Added autoprefixer only for apos builds as dicsussed

ValJed · 2024-11-05T16:08:35Z

lib/vite-postcss-config.js

+
+  if (!Array.isArray(plugins)) {
+    util.error('WARNING: postcss.config.js must export an array of plugins');
+  }


We force the user to use plugins as arrays like described in the doc, otherwise making the require from here would not work.
Like this:

import autoprefixer from 'autoprefixer'; import tailwindcss from 'tailwindcss'; export default { plugins: [ autoprefixer(), tailwindcss() ] };

Not like this:

export default { plugins: { autoprefixed: {}, tailwindcss: {} } };

Look at my recommendation (HUGE) comment. Yes, this is a great idea and I also think we should throw and not only warn. The reason for that is:

If a project transitions to our Vite build and already has a postcss config due to internal other use of potscss, the project developers should be aware that there is a conflcit and they are breaking the Apostrophe build pipeline.

They can make a decision how to proceed because they know stuff is broken on build time, which is GOOD.

I also think we need to ping Bob so that he can add important warning boxes in the docs about the whole possible postcss configuration conflicts and requirements - this one can be cited from the Vite docs, link in my previous comment.

Both versions are working with the loader now.

ValJed · 2024-11-05T16:09:21Z

package.json

@@ -35,6 +35,7 @@
  "dependencies": {
    "@vitejs/plugin-vue": "^5.1.4",
    "fs-extra": "^7.0.1",
+    "postcss-viewport-to-container-toggle": "github:apostrophecms/postcss-viewport-to-container-toggle#pro-6694-mobile-preview-plugin",


TODO: update when plugin released.

myovchev

I have outlined a more detailed strategy for implementing postcss by the Vite standards, preserving the experience that devs are used to. I also think a core patch is also required (to pass pre-processed mobile preview related options, translated to a "build module language"). This is important as we might want to have webpack build module one day and the whole doesn't scale well - any core internal change will break build modules.

myovchev · 2024-11-06T12:33:18Z

lib/internals.js

        sourceRoot: self.buildRootSource,
        input: self.getBuildInputs('apos')
      });
+      const postcssConfig = await vitePostcssConfig({


A better implementation of this would be:

Move the entire postcss config processing in its own async getPostcssConfig(options) method.

Inside of this method, detect the apos.assset.options and just send simple instructions to the config. Is the plugin enabled? Pass the configuration options that the config may pass directly to the plugin. Or even better, use the build options (see below) - they are cached in self.buildOptions.

What I think is really bad here, is that Vite module now knows Apostrophe core internals. Something I wanted to avoid. Vite module only cares about building. It shouldn't know anything about "mobile preview" or any other feature and business logic. We should decouple. So I really recommend to patch the core, find getBuildOptions() core asset method, and add additional build options, something like:

{ postcssTransformViewport: { enable: self.options.breakpointPreviewMode?.enable, debug: self.options.breakpointPreviewMode?.debug === true, attr: 'data-breakpoint-preview-mode', hook: self.options.breakpointPreviewMode?.transform }

Pass those to await getPostcssConfig({ ...self.buildOptions.postcssTransformViewport })

Remove all the business logic from vite-postcss-config.js. The config should only check if the plugin is needed, if yes load it and pass blindly the arguments. It should not perform any merges as well, a plain object containing only the known postcss plugin.

Back to getPostcssConfig, it loads the base postcss config using the factory function as it does now (but with the simple options described above). It also should take care of detecting the project level postcss config file (if any). And this is a no small task! More about it below. In the end it should use the Vite merge helper to merge our base config and loaded postcss config:

const finalPostcssConfig = await vite.mergeConfig(baseConfig, { css: { postcss: userConfig } });

Loading the project level postcss config might be its own method as well. Reference to vite docs: https://vite.dev/config/shared-options.html#css-postcss. It shows that Vite itself attempts loading the config using a plugin - https://github.com/postcss/postcss-load-config?tab=readme-ov-file. And it's a pretty complex logic! I think our best move here is to use internally this plugin (it exports a single async function) and pass the proper configuration to it. It's pretty simple code, so implement this shouldn't be a problem. Note - use self.apos.rootDir as a base config location, because process.cwd() will break multisite.

In the end, make sure that loading user (project level) postcss config in getPostcssConfig happens conditionally based on an option. Switch this option off in getAposViteConfig, and on in getPublicViteConfig.

In the end, I would also recommend make the postcss plugin ESM module. I'm not sure what are the reasons for you to go back to CJS, but using a dynamic import() inside of the config file is OK and already done for our own plugin. The config factories are all async by design.

myovchev · 2024-11-06T12:37:32Z

lib/vite-postcss-config.js

+
+  if (!Array.isArray(plugins)) {
+    util.error('WARNING: postcss.config.js must export an array of plugins');
+  }


Look at my recommendation (HUGE) comment. Yes, this is a great idea and I also think we should throw and not only warn. The reason for that is:

If a project transitions to our Vite build and already has a postcss config due to internal other use of potscss, the project developers should be aware that there is a conflcit and they are breaking the Apostrophe build pipeline.

They can make a decision how to proceed because they know stuff is broken on build time, which is GOOD.

I also think we need to ping Bob so that he can add important warning boxes in the docs about the whole possible postcss configuration conflicts and requirements - this one can be cited from the Vite docs, link in my previous comment.

myovchev · 2024-11-06T12:41:20Z

lib/vite-postcss-config.js

+  const postCssConfigPath = path.resolve(process.cwd(), 'postcss.config.js');
+  const importedConfig = isPublic && fs.existsSync(postCssConfigPath)
+    ? await import(postCssConfigPath)
+    : {};


Autoprefixer for Apos might make sense. I don't think Vite does anything related, I can't recall they are doing it by default (probably inspecting the relevant docs/research a bit is a good idea). If we decide to do that, you can add it directly in the apos base config (vite-apos-config.js). It will be merged with anything that potentially comes later.

myovchev · 2024-11-06T12:41:38Z

lib/vite-postcss-config.js

+  const {
+    plugins = [],
+    ...postCssConfig
+  } = importedConfig.default || importedConfig;


We need the official postcss loader.

…dds autoprefixer for apos builds

ValJed · 2024-11-06T16:20:54Z

lib/internals.js

        sourceRoot: self.buildRootSource,
        input: self.getBuildInputs('apos')
      });
+      const postcssConfig = await self.getPostcssConfig(self.buildOptions, 'apos');
+      const aposConfig = vite.mergeConfig(config, postcssConfig);


looks a bit messy to chain the merge configs but it should work well.

ValJed · 2024-11-06T16:21:43Z

lib/internals.js

+      }
+
+      return vitePostcssConfig({ plugins: postcssPlugins });
+    },


Moved the logic here, tried the respect the plugins order. options coming from core.

myovchev

LGTM

ValJed added 2 commits November 4, 2024 16:49

uses new postcss plugin for mobile preview in base config

788046a

adds postcss plugin only if breakpoint preview is enabled

8d76f1c

ValJed added 8 commits November 4, 2024 16:59

adds changelog

1b00f00

moves postcss in dependencies

6f0178d

removes postcss config from base config

e25ab6d

adds vite-postcss-config file

a67a9a7

generates postcss config for public

2b47132

updates getPublicViteConfig function

d6a7ffe

supportes postcss for apos builds

765e15e

removes unused code in base config

f06bcb0

ValJed force-pushed the pro-6694-breakpoint-preview-vite-ready branch from fd4e113 to f06bcb0 Compare November 5, 2024 16:04

ValJed commented Nov 5, 2024

View reviewed changes

ValJed requested a review from myovchev November 5, 2024 16:16

ValJed marked this pull request as draft November 5, 2024 16:22

myovchev requested changes Nov 6, 2024

View reviewed changes

ValJed added 3 commits November 6, 2024 17:17

adds postcss-load-config and autoprefixer

d0f036d

moves logic in getPostcssConfig, uses postcss loader to get config, a…

0bea5ce

…dds autoprefixer for apos builds

updates changelog

797c623

ValJed commented Nov 6, 2024

View reviewed changes

ValJed requested a review from myovchev November 6, 2024 16:22

myovchev approved these changes Nov 7, 2024

View reviewed changes

ValJed added 2 commits November 7, 2024 11:09

targets main branch of postcss plugin

b652a74

targets plugin version

a263286

ValJed marked this pull request as ready for review November 7, 2024 11:21

ValJed merged commit be412d9 into main Nov 7, 2024
4 checks passed

ValJed deleted the pro-6694-breakpoint-preview-vite-ready branch November 7, 2024 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pro 6694 breakpoint preview vite ready #9

Pro 6694 breakpoint preview vite ready #9

ValJed commented Nov 4, 2024 •

edited

Loading

linear bot commented Nov 4, 2024

ValJed Nov 5, 2024

myovchev Nov 6, 2024

ValJed Nov 6, 2024

ValJed Nov 5, 2024

myovchev Nov 6, 2024

ValJed Nov 6, 2024

ValJed Nov 5, 2024

myovchev Nov 6, 2024

ValJed Nov 6, 2024

ValJed Nov 5, 2024

myovchev left a comment

myovchev Nov 6, 2024

myovchev Nov 6, 2024

myovchev Nov 6, 2024

myovchev Nov 6, 2024

ValJed Nov 6, 2024

ValJed Nov 6, 2024

myovchev left a comment

Pro 6694 breakpoint preview vite ready #9

Pro 6694 breakpoint preview vite ready #9

Conversation

ValJed commented Nov 4, 2024 • edited Loading

Summary

What are the specific steps to test this change?

What kind of change does this PR introduce?

Make sure the PR fulfills these requirements:

linear bot commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myovchev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myovchev left a comment

Choose a reason for hiding this comment

ValJed commented Nov 4, 2024 •

edited

Loading