-
-
Notifications
You must be signed in to change notification settings - Fork 356
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
Streamlining production of tailwindcss bundled css, introducing multiple entryfiles for content-ui foldesr #750
base: main
Are you sure you want to change the base?
Changes from 3 commits
2ae1052
320ce7e
7357f4e
9fb467b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because of missing implementation about shadow dom injection, this is totally not working right now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 you couldn't see it, because of the fail build? did you try local version? |
||
}, | ||
{ | ||
matches: ['http://*/*', 'https://*/*', '<all_urls>'], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
export * from './watch-rebuild-plugin'; | ||
export * from './make-entry-point-plugin'; | ||
export * from './watch-public-plugin'; | ||
export * from './tailwindcss-bundler'; |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,42 @@ | ||||||||||
import { Plugin } from 'vite'; | ||||||||||
import fs from 'fs/promises'; | ||||||||||
import path from 'path'; | ||||||||||
Comment on lines
+2
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ohh node must be included? what happen if don't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||
import postcss from 'postcss'; | ||||||||||
import tailwindcss from 'tailwindcss'; | ||||||||||
import autoprefixer from 'autoprefixer'; | ||||||||||
import postcssImport from 'postcss-import'; | ||||||||||
import LightningCSS from 'lightningcss'; | ||||||||||
|
||||||||||
const rootDir = path.resolve(__dirname, '../../../../'); | ||||||||||
|
||||||||||
export function processCssPlugin(): Plugin { | ||||||||||
return { | ||||||||||
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'); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's really necessary to change name to |
||||||||||
|
||||||||||
const css = await fs.readFile(inputFile, 'utf-8'); | ||||||||||
const result = await postcss([postcssImport(), tailwindcss, autoprefixer]).process(css, { | ||||||||||
from: inputFile, | ||||||||||
to: outputFile, | ||||||||||
}); | ||||||||||
|
||||||||||
const minified = LightningCSS.transform({ | ||||||||||
code: Buffer.from(result.css, 'utf8'), | ||||||||||
minify: true, | ||||||||||
targets: { | ||||||||||
// Define your target browsers or leave empty for default | ||||||||||
}, | ||||||||||
filename: 'global.css', | ||||||||||
}); | ||||||||||
|
||||||||||
const finalCss = Buffer.from(minified.code).toString('utf8'); | ||||||||||
|
||||||||||
await fs.mkdir(path.dirname(outputFile), { recursive: true }); | ||||||||||
await fs.writeFile(outputFile, finalCss); | ||||||||||
|
||||||||||
console.log('CSS processed and saved to', outputFile); | ||||||||||
}, | ||||||||||
}; | ||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,9 +14,9 @@ | |
"clean:node_modules": "pnpx rimraf node_modules", | ||
"clean:turbo": "rimraf .turbo", | ||
"clean": "pnpm clean:bundle && pnpm clean:node_modules && pnpm clean:turbo", | ||
"genenrate-i8n": "node genenrate-i18n.mjs", | ||
"ready": "pnpm genenrate-i8n && node build.dev.mjs", | ||
"build": "pnpm genenrate-i8n && node build.prod.mjs", | ||
"generate-i8n": "node generate-i18n.mjs", | ||
"ready": "pnpm generate-i8n && node build.dev.mjs", | ||
"build": "pnpm generate-i8n && node build.prod.mjs", | ||
Comment on lines
+17
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍🏼 |
||
"lint": "eslint . --ext .ts,.tsx", | ||
"lint:fix": "pnpm lint --fix", | ||
"prettier": "prettier . --write --ignore-path ../../.prettierignore", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
@tailwind base; | ||
@tailwind components; | ||
@tailwind utilities; | ||
@tailwind utilities; |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't work that way, it need to edit This should reading all folders from e.g. |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,52 @@ | ||||||
import { readdirSync, existsSync, mkdirSync, writeFileSync, Dirent, readFileSync, renameSync } from 'fs'; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
import { resolve } from 'node:path'; | ||||||
|
||||||
const rootDir = resolve(__dirname, '..'); | ||||||
const srcDir = resolve(rootDir, 'src'); | ||||||
|
||||||
export const getEntryPoints = (options: { include?: string[]; exclude?: string[]; createRecursively?: boolean }) => { | ||||||
const entryPoints: Record<string, string> = {}; | ||||||
|
||||||
const foldersToProcess = | ||||||
options.include || | ||||||
readdirSync(srcDir, { withFileTypes: true }) | ||||||
.filter((item): item is Dirent => item.isDirectory()) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not like that?
Suggested change
I don't really know this kind of typing, that's interesting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an unnecessary assertion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
erm we only want folder name, and not file names, readdirsync will read filenames too which we don't want. |
||||||
.map(item => item.name); | ||||||
|
||||||
foldersToProcess.forEach(folderName => { | ||||||
const shouldExclude = options.exclude && options.exclude.includes(folderName); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe sth like that:
Suggested change
|
||||||
|
||||||
if (!shouldExclude) { | ||||||
const entryFolder = resolve(srcDir, folderName); | ||||||
const entryFileTS = resolve(entryFolder, 'index.ts'); | ||||||
const entryFileTSX = resolve(entryFolder, `${folderName}.tsx`); | ||||||
const existingEntryFileTSX = resolve(srcDir, 'App.tsx'); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
|
||||||
if (!existsSync(entryFolder) && options.createRecursively) { | ||||||
mkdirSync(entryFolder, { recursive: true }); | ||||||
writeFileSync(entryFileTS, `export * from "./${folderName}";`); | ||||||
|
||||||
// Copy the existing entry file to the new entry file, just as a template | ||||||
if (existsSync(existingEntryFileTSX)) { | ||||||
writeFileSync(entryFileTSX, readFileSync(existingEntryFileTSX, 'utf8')); | ||||||
|
||||||
try { | ||||||
// renameSync(existingEntryFileTSX, resolve(srcDir, 'App-deleteMe!.tsx')); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't leave comments like this for review |
||||||
} catch (error) { | ||||||
console.error('Failed to rename file:', error); | ||||||
} | ||||||
} else { | ||||||
writeFileSync(entryFileTSX, ''); | ||||||
} | ||||||
} | ||||||
|
||||||
if (existsSync(entryFolder)) { | ||||||
entryPoints[folderName] = entryFileTS; | ||||||
} else { | ||||||
console.warn(`Folder "${folderName}" does not exist and was not created.`); | ||||||
} | ||||||
} | ||||||
}); | ||||||
|
||||||
return entryPoints; | ||||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export * from './page1'; |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeap i agree. i just want to show example |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
import { useEffect } from 'react'; | ||
import { Button } from '@extension/ui'; | ||
import { useStorage } from '@extension/shared'; | ||
import { exampleThemeStorage } from '@extension/storage'; | ||
|
||
export default function App() { | ||
const theme = useStorage(exampleThemeStorage); | ||
|
||
useEffect(() => { | ||
console.log('content ui loaded'); | ||
}, []); | ||
|
||
return ( | ||
<div className="flex items-center justify-between gap-2 bg-blue-100 rounded py-1 px-2"> | ||
<div className="flex gap-1 text-blue-500"> | ||
Edit <strong className="text-blue-700">pages/content-ui/src/app.tsx</strong> and save to reload. | ||
</div> | ||
<Button theme={theme} onClick={exampleThemeStorage.toggle}> | ||
Toggle Theme | ||
</Button> | ||
</div> | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export * from './page2'; |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
import { useEffect } from 'react'; | ||
import { Button } from '@extension/ui'; | ||
import { useStorage } from '@extension/shared'; | ||
import { exampleThemeStorage } from '@extension/storage'; | ||
|
||
export default function App() { | ||
const theme = useStorage(exampleThemeStorage); | ||
|
||
useEffect(() => { | ||
console.log('content ui loaded'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't be the same as in first script, but anyway. |
||
}, []); | ||
|
||
return ( | ||
<div className="flex items-center justify-between gap-2 bg-blue-100 rounded py-1 px-2"> | ||
<div className="flex gap-1 text-blue-500"> | ||
Edit <strong className="text-blue-700">pages/content-ui/src/app.tsx</strong> and save to reload. | ||
</div> | ||
<Button theme={theme} onClick={exampleThemeStorage.toggle}> | ||
Toggle Theme | ||
</Button> | ||
</div> | ||
); | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,24 +1,38 @@ | ||
import { resolve } from 'node:path'; | ||
import { makeEntryPointPlugin } from '@extension/hmr'; | ||
import { isDev, withPageConfig } from '@extension/vite-config'; | ||
import { processCssPlugin } from '@extension/hmr'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
import { getEntryPoints } from './script/entryPoints'; | ||
|
||
const rootDir = resolve(__dirname); | ||
const srcDir = resolve(rootDir, 'src'); | ||
|
||
/** | ||
* 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 | ||
}); | ||
Comment on lines
+9
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems more like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
export default withPageConfig({ | ||
resolve: { | ||
alias: { | ||
'@src': srcDir, | ||
}, | ||
}, | ||
plugins: [isDev && makeEntryPointPlugin()], | ||
plugins: [processCssPlugin()], | ||
publicDir: resolve(rootDir, 'public'), | ||
build: { | ||
lib: { | ||
entry: resolve(srcDir, 'index.tsx'), | ||
entry: entryPoints, | ||
name: 'contentUI', | ||
formats: ['iife'], | ||
fileName: 'index', | ||
formats: ['es'], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't have it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm my repo worked fine, but im really puzzled how the test all failed, i didn't even touched the content-runtime stuff There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Try to e.g. inject runtime script several times, you will have an error |
||
fileName: (format, entryName) => | ||
isDev ? `${entryName}/index_dev.js` : `${entryName}/index.js` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are you need to have 2 different files? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. filename should be different in dev and production mode, perhaps hashes should be applied in production? that's the convention no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, i don't think it's really necessary to add prefix |
||
}, | ||
outDir: resolve(rootDir, '..', '..', 'dist', 'content-ui'), | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can't be ignored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops i forgot ive done that, will remove it.. i just have to push another commit right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's your normal workflow, PR it's only for tracking it