Streamlining production of tailwindcss bundled css, introducing multiple entryfiles for content-ui foldesr #750

tngflx · 2024-09-18T06:23:38Z

Make tailwindcss centralized css production settings, without creating postcss config in every package.json
Added multiple entryfiles just for content-ui

Priority*

High: This PR needs to be merged first for other tasks.
Middle: This PR should be merged quickly to prevent conflicts due to common changes. (default)
Low: This PR does not affect other tasks, so it can be merged later.

Purpose of the PR*

Streamlining css production for shadowdom, which included minification and treeshaking to be used for shadowdom
Added multiple entryfiles just for content-ui

Changes*

Tailwindcss bundle production is a mess, i created a plugin to be shared accross all pages folder, without the need of redundant @tailwind base ... etc, to be pasted all over the code, the plugin itself contains the postcss config, and thereby internalizing the css production for shadowdom. And hence the command https://github.com/Jonghakseo/chrome-extension-boilerplate-react-vite/blob/a7d51c3266803703e211223e49de675d119fdfa6/pages/content-ui/package.json#L15C1-L16C1 can be removed,
and this also fixes hmr crashes upon live editing of tailwindcss classess
Multi entry files for content-ui folder, as a normal dev will have multiple subdomains to inject across, its smarter to have multiple entry files injected for different subdomains, I've made the script to autogenerate folders and files inside content-ui, but u have to pay attention as well to make changes in manifest.js

How to check the feature

Everything works even without the postcss config in package.json, and every pages now have its own generated global-out.css

just fork my repo and install as usual, you'll see all the tailwindcss still works perfectly

https://discord.com/channels/1263404974830915637/1264514968288759889/1285558712941023345
as you see above hmr doesn't crash

All the css is still working but with leaner size

…e output

2) fixed hmr crash with internalizing tailwindcss build file 3) added multi entrypoints for content-ui folder only 4) added tailwindcss-bundler to be shared across pages

PatrykKuniczak · 2024-09-20T13:48:43Z

.gitignore

+**/vite.config.mts.timestamp-*
+pnpm-lock.yaml


This can't be ignored

oops i forgot ive done that, will remove it.. i just have to push another commit right?

Yeah, that's your normal workflow, PR it's only for tracking it

PatrykKuniczak · 2024-09-20T13:51:22Z

packages/hmr/lib/plugins/tailwindcss-bundler.ts

+import fs from 'fs/promises';
+import path from 'path';


Suggested change

import fs from 'fs/promises';

import path from 'path';

import fs from 'node:fs/promises';

import path from 'node:path';

ohh node must be included? what happen if don't

https://dev.to/yenyih/new-nodejs-protocol-imports-3pnf

PatrykKuniczak · 2024-09-20T14:04:03Z

pages/content-ui/script/entryPoints.ts

+          writeFileSync(entryFileTSX, readFileSync(existingEntryFileTSX, 'utf8'));
+
+          try {
+            // renameSync(existingEntryFileTSX, resolve(srcDir, 'App-deleteMe!.tsx'));


Don't leave comments like this for review

PatrykKuniczak · 2024-09-20T18:10:26Z

pages/content-ui/script/entryPoints.ts

+      .map(item => item.name);
+
+  foldersToProcess.forEach(folderName => {
+    const shouldExclude = options.exclude && options.exclude.includes(folderName);


Maybe sth like that:

Suggested change

const shouldExclude = options.exclude && options.exclude.includes(folderName);

const shouldExclude = options.exclude?.includes(folderName);

PatrykKuniczak · 2024-09-20T18:12:03Z

pages/content-ui/script/entryPoints.ts

+  const foldersToProcess =
+    options.include ||
+    readdirSync(srcDir, { withFileTypes: true })
+      .filter((item): item is Dirent => item.isDirectory())


Why not like that?

Suggested change

.filter((item): item is Dirent => item.isDirectory())

.filter((item: : Dirent) => item.isDirectory())

I don't really know this kind of typing, that's interesting

This is an unnecessary assertion.

This is an unnecessary assertion.

erm we only want folder name, and not file names, readdirsync will read filenames too which we don't want.

PatrykKuniczak · 2024-09-20T18:25:49Z

pages/content-ui/src/page2/page2.tsx

+  const theme = useStorage(exampleThemeStorage);
+
+  useEffect(() => {
+    console.log('content ui loaded');


Shouldn't be the same as in first script, but anyway.
1 example is enough

PatrykKuniczak · 2024-09-20T18:29:36Z

pages/content-ui/vite.config.mts

      name: 'contentUI',
-      formats: ['iife'],
-      fileName: 'index',
+      formats: ['es'],


Don't have it to es because it isn't working good, i've checked it XD

hmm my repo worked fine, but im really puzzled how the test all failed, i didn't even touched the content-runtime stuff

Try to e.g. inject runtime script several times, you will have an error

PatrykKuniczak · 2024-09-20T18:31:57Z

pages/content-ui/vite.config.mts

-      fileName: 'index',
+      formats: ['es'],
+      fileName: (format, entryName) =>
+        isDev ? `${entryName}/index_dev.js` : `${entryName}/index.js`


Why are you need to have 2 different files?

filename should be different in dev and production mode, perhaps hashes should be applied in production? that's the convention no?

No, i don't think it's really necessary to add prefix
If there's some differences between dev and prod versions, let's add if statement or sth inside code, and you'll have other output file.

PatrykKuniczak · 2024-09-20T18:34:35Z

pages/new-tab/src/index.tsx

@@ -1,6 +1,6 @@
 import { createRoot } from 'react-dom/client';
 import '@src/index.css';
-import '@extension/ui/lib/global.css';
+import '../dist/global-out.css';


Maybe your plugin are able to add this import during build to .js bundled file?
We can avoid those imports in each file.

huh that's a great idea, perhaps will solve my build time error not found this file during build

PatrykKuniczak · 2024-09-20T18:35:48Z

packages/hmr/lib/plugins/tailwindcss-bundler.ts

+    name: 'process-css',
+    async configResolved(config) {
+      const inputFile = path.resolve(rootDir, 'ui/lib/global.css');
+      const outputFile = path.resolve(config.root, 'dist/global-out.css');


It's really necessary to change name to -out?

Jonghakseo

There are three changes to this PR

centralization of tailwindcss bundling
add multiple entry points for content-ui
update package (?)

First of all, the centralization of the CSS bundling process into the build process seems like a good idea. Basically I support your idea, but I'm concerned that we need to make sure there are no side effects or unexpected issues, that it's not a common implementation and users may struggle when adding the plugin from existing settings(by some tutorials, guide...), and that it's difficult to set the settings separately on each page.

Second, I'm skeptical that adding an entry point for content-ui is a necessary task. Users can achieve the same thing by copy/pasting the pages they want, such as content, content-ui, and registering them in the manifest if they want. My understanding is that this is a convenience feature, is that correct? Please let me know if I'm missing something.

Jonghakseo · 2024-09-21T04:24:06Z

packages/i18n/package.json

+    "generate-i8n": "node generate-i18n.mjs",
+    "ready": "pnpm generate-i8n && node build.dev.mjs",
+    "build": "pnpm generate-i8n && node build.prod.mjs",


Jonghakseo · 2024-09-21T04:26:46Z

pages/content-ui/src/page1/page1.tsx

This should be page1/App.tsx

yeap i agree. i just want to show example

Jonghakseo · 2024-09-21T04:27:17Z

pages/content-ui/src/page2/page2.tsx

page2/App.tsx 👀

Jonghakseo · 2024-09-21T04:29:21Z

pages/content-ui/vite.config.mts

 import { isDev, withPageConfig } from '@extension/vite-config';
+import { processCssPlugin } from '@extension/hmr';


IMO some people might wonder why this plugin include hmr package... 🤔

It would be nice to refactor it to collect packages used in the development environment in the future.

erm bro this is your own method ... haha you've written like this for your own plugin, so i just follow your convention :

import { makeEntryPointPlugin } from '@extension/hmr'; import { withPageConfig, isDev } from '@extension/vite-config'; const rootDir = resolve(__dirname); const libDir = resolve(rootDir, 'lib'); export default withPageConfig({ resolve: { alias: { '@lib': libDir, }, }, publicDir: resolve(rootDir, 'public'), plugins: [isDev && makeEntryPointPlugin()],

i follow your makeentrypointplugin

Jonghakseo · 2024-09-21T05:01:26Z

pages/content-ui/vite.config.mts

+/**
+ * Here is the place to add new entry points, u can straight away add new folders here to include
+ * without creating folders in src dir manually.
+ * @param {string[]} options.include - Folders to include !!MODIFY ME!!
+ * @param {string[]} options.exclude - Folders to exclude !!MODIFY ME!!
+ * @param {boolean} options.createRecursively - Create folders recursively
+ * @returns {Record<string, string>}
+ */
+const entryPoints = getEntryPoints({ 
+  include: ['page1', 'page2'], createRecursively: true 
+});


It seems more like makeEntryponits than getEntryPoints, and shouldn't it be possible to run it as a separate script? I'm curious why it's in the vite config together.

cause getentrypoints/makeentrypoints is resolving to the array of folder names to be used in vite.config entries. that's why its here. if not vite config don't know which folder to process. And yes this is more of convenience feature and people can add to manifest.json later

Jonghakseo · 2024-09-21T05:03:53Z

pages/content-ui/script/entryPoints.ts

+  const foldersToProcess =
+    options.include ||
+    readdirSync(srcDir, { withFileTypes: true })
+      .filter((item): item is Dirent => item.isDirectory())


This is an unnecessary assertion.

Jonghakseo · 2024-09-21T05:07:07Z

pages/content-ui/script/entryPoints.ts

+      const entryFolder = resolve(srcDir, folderName);
+      const entryFileTS = resolve(entryFolder, 'index.ts');
+      const entryFileTSX = resolve(entryFolder, `${folderName}.tsx`);
+      const existingEntryFileTSX = resolve(srcDir, 'App.tsx');


The details of creating an existing App.tsx as an entry point for pages is a very specific implementation that is difficult to understand from the outside.

App.tsx is not currently used anywhere, so it seems like a good idea to rename it to something like Template.tsx.

ahhh i just added that, cause user can choose between just single page or multi page, if user chooses to have multipage, he can always just add stuff into getentrypoints.includes which will generate those template for them. so its easier for them to understand.

Jonghakseo · 2024-09-21T05:08:30Z

chrome-extension/manifest.js

@@ -51,7 +51,7 @@ const manifest = deepmerge(
      },
      {
        matches: ['http://*/*', 'https://*/*', '<all_urls>'],
-        js: ['content-ui/index.iife.js'],
+        js: ['content-ui/page1/index_dev.js'],


Because of missing implementation about shadow dom injection, this is totally not working right now

erm bro.. basically what you need to do is just this :

import { createRoot } from 'react-dom/client'; import '@src/index.css'; import '../dist/global-out.css'; import NewTab from '@src/NewTab';

ive included in every page that needed this css bundle.. it worked outright for me without pasting
@tailwind base @tailwind components all over the place

you couldn't see it, because of the fail build? did you try local version?

tngflx · 2024-09-21T06:34:48Z

There are three changes to this PR

centralization of tailwindcss bundling

add multiple entry points for content-ui

update package (?)

First of all, the centralization of the CSS bundling process into the build process seems like a good idea. Basically I support your idea, but I'm concerned that we need to make sure there are no side effects or unexpected issues, that it's not a common implementation and users may struggle when adding the plugin from existing settings(by some tutorials, guide...), and that it's difficult to set the settings separately on each page.

Second, I'm skeptical that adding an entry point for content-ui is a necessary task. Users can achieve the same thing by copy/pasting the pages they want, such as content, content-ui, and registering them in the manifest if they want. My understanding is that this is a convenience feature, is that correct? Please let me know if I'm missing something.

based on ur first question, since the original code was already opinionated on tailwindcss i just help u to internalize the bundle process which is the exact same as running the build:tailwindcss script, your approach doesnt provide hmr and mine does. we can ofc work on different ui package which people ccan just install whtever ui library they want. but the step of bundling should be the same

tngflx added 3 commits September 17, 2024 20:28

added multi entryfiles for content-ui + internalize tailwindcss bundl…

2ae1052

…e output

fixed postcss-import error

320ce7e

1) removed unnecessary postcss config in package.json

7357f4e

2) fixed hmr crash with internalizing tailwindcss build file 3) added multi entrypoints for content-ui folder only 4) added tailwindcss-bundler to be shared across pages

tngflx requested a review from Jonghakseo as a code owner September 18, 2024 06:23

PatrykKuniczak requested changes Sep 20, 2024

View reviewed changes

Jonghakseo reviewed Sep 21, 2024

View reviewed changes

tngflx force-pushed the main branch from 6d901de to 7357f4e Compare September 29, 2024 08:36

Merge branch 'Jonghakseo:main' into main

9fb467b

	const shouldExclude = options.exclude && options.exclude.includes(folderName);
	const shouldExclude = options.exclude?.includes(folderName);

	.filter((item): item is Dirent => item.isDirectory())
	.filter((item: : Dirent) => item.isDirectory())

		import { isDev, withPageConfig } from '@extension/vite-config';
		import { processCssPlugin } from '@extension/hmr';

		*/vite.config.mts.timestamp-
		pnpm-lock.yaml

Streamlining production of tailwindcss bundled css, introducing multiple entryfiles for content-ui foldesr #750

Are you sure you want to change the base?

Streamlining production of tailwindcss bundled css, introducing multiple entryfiles for content-ui foldesr #750

Conversation

tngflx commented Sep 18, 2024

Priority*

Purpose of the PR*

Changes*

How to check the feature

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

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

Choose a reason for hiding this comment

Jonghakseo 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

tngflx Sep 21, 2024 • edited Loading

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

tngflx Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

tngflx commented Sep 21, 2024

tngflx Sep 21, 2024 •

edited

Loading

tngflx Sep 21, 2024 •

edited

Loading