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.

Sign up for GitHub

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(theme-tool): Reimplementing the theme-tool method #2369

Merged
merged 6 commits into from
Oct 24, 2024
Merged

refactor(theme-tool): Reimplementing the theme-tool method #2369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
46ea9ed
fix(theme): fix
shenjunjian Oct 22, 2024
73c9891
refactor(theme-tool): refactor theme-tool Class, adopt style by adopt…
shenjunjian Oct 23, 2024
b1e6e75
fix(theme): fix
shenjunjian Oct 23, 2024
9a61f5b
refactor(theme-tool): refactor theme-tool done
shenjunjian Oct 23, 2024
b025277
fix(theme): fix
shenjunjian Oct 23, 2024
4684771
fix(theme-tool): support multi target
shenjunjian Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ module.exports = {
'@typescript-eslint/no-use-before-define': 'off',
'@typescript-eslint/restrict-template-expressions': 'off',
'@typescript-eslint/no-invalid-this': 'off',
'vue/no-deprecated-dollar-scopedslots-api': 'off'
'vue/no-deprecated-dollar-scopedslots-api': 'off',
'@typescript-eslint/lines-between-class-members': 'off'
}
}
10 changes: 8 additions & 2 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,18 @@ tgzs
*.tgz


# 以下用不到了
packages/theme/scripts/theme.json
packages/theme/scripts/theme-result.txt
packages/theme/scripts/themeExcel.xlsx

packages/theme/src/theme/*-theme/component.js
packages/theme/src/aurora-theme
packages/theme/src/smb-theme
#------------------------------
Comment on lines +62 to +69
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Theme files have been relocated, not removed

The verification reveals that the theme-related files and directories have been moved from packages/theme/ to packages/theme-saas/:

  • theme.json exists at packages/theme-saas/theme/theme.json
  • Aurora and SMB themes exist under packages/theme-saas/src/theme/

This suggests a restructuring of theme-related code rather than removal. The .gitignore entries should be updated to reflect the new locations if these files still need to be ignored.

Recommended actions:

  • Update .gitignore entries to point to the new locations under packages/theme-saas/ if these files should still be ignored
  • Remove the old entries as they no longer reflect the current codebase structure
🔗 Analysis chain

Verify removal of theme-related files from codebase.

The removal of these entries from .gitignore suggests these files are no longer part of the codebase. Let's verify this to prevent potential issues.

Let me generate additional scripts to verify if the theme-related directories and files exist in any form within the repository, as the previous script showed that the specific paths don't exist but we should check if they were moved elsewhere.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the removed theme files still exist in the repository
# Expect: No matches, indicating files have been properly removed

echo "Checking for removed theme files..."
for path in \
  "packages/theme/scripts/theme.json" \
  "packages/theme/scripts/theme-result.txt" \
  "packages/theme/scripts/themeExcel.xlsx" \
  "packages/theme/src/aurora-theme" \
  "packages/theme/src/smb-theme"; do
  if [ -e "$path" ]; then
    echo "Warning: $path still exists"
  fi
done

# Check for any theme component.js files
fd "component.js" "packages/theme/src/theme" --type f

Length of output: 1473

Script:

#!/bin/bash
# Check if any of these files/patterns exist anywhere in the repository
echo "Searching for theme-related files..."

# Search for theme.json files
echo "=== theme.json files ==="
fd "theme.json" --type f

# Search for theme-result.txt files
echo -e "\n=== theme-result.txt files ==="
fd "theme-result.txt" --type f

# Search for themeExcel.xlsx files
echo -e "\n=== themeExcel.xlsx files ==="
fd "themeExcel.xlsx" --type f

# Search for aurora-theme directory
echo -e "\n=== aurora-theme directories/files ==="
fd "aurora-theme"

# Search for smb-theme directory
echo -e "\n=== smb-theme directories/files ==="
fd "smb-theme"

# Search for component.js files under any theme directory
echo -e "\n=== theme/*-theme/component.js patterns ==="
fd "component.js" --type f | grep -i "theme.*component\.js" || true

Length of output: 884


# theme 的汇总文件是自动生成的
packages/theme/src/old-theme-index.less
packages/theme/src/index.less

pnpm-lock.yaml
gulp/bundle.json
Expand All @@ -79,4 +84,5 @@ test-results
examples/sites/public/tiny-vue*.js
examples/sites/public/tiny-vue*.mjs
examples/sites/public/tailwind.css
examples/sites/public/index.css
examples/sites/public/index.css

4 changes: 2 additions & 2 deletions packages/theme/build/gulp-dist.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ function concatIndex(cb) {
}
// 2、拼接所有组件的 old-theme.less 到一起 old-theme-index.less
function concatOldTheme(cb) {
_concatFiles('../src/*/old-theme.less', '../src/old-theme.less')
_concatFiles('../src/*/old-theme.less', '../src/old-theme-index.less')
cb()
}
// 3、编译
Expand All @@ -41,7 +41,7 @@ gulp.task('compile', () => {
.src([
`${source}/**/index.less`, // 编译默认主题
`${source}/index.less`,
`${source}/old-theme.less` // 编译旧主题
`${source}/old-theme-index.less` // 编译旧主题
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding source maps for better debugging.

While the path update is correct, the build process could benefit from source maps to aid debugging in development.

Add source map generation to the build pipeline:

 gulp.task('compile', () => {
   return gulp
     .src([
       `${source}/**/index.less`,
       `${source}/index.less`,
       `${source}/old-theme-index.less`
     ])
+    .pipe(sourcemaps.init())
     .pipe(svgInline(svgInlineOption))
     .pipe(less())
     .pipe(prefixer({
       borwsers: ['last 1 version', '> 1%', 'not ie <= 8'],
       cascade: true,
       remove: true
     }))
     .pipe(svgInline(svgInlineOption))
     .pipe(cssmin())
+    .pipe(sourcemaps.write('./'))
     .pipe(gulp.dest(dist))
 })

This would require adding gulp-sourcemaps to your dependencies:

import sourcemaps from 'gulp-sourcemaps'

])
.pipe(svgInline(svgInlineOption))
.pipe(less())
Expand Down
27 changes: 23 additions & 4 deletions packages/theme/build/release.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,31 @@
import fs from 'node:fs'
import path from 'node:path'

// 复制package.json/README.md到dist目录
const root = path.resolve('./')

// 1、复制 theme-tool.js /README.md 到dist目录
fs.copyFileSync('README.md', path.join('dist', 'README.md'))
fs.copyFileSync('src/theme-tool.js', path.join('dist', 'theme-tool.js'))
fs.copyFileSync('src/theme-tool.d.ts', path.join('dist', 'theme-tool.d.ts'))
Comment on lines +6 to +9
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling for file operations.

The synchronous file operations could throw errors if source files don't exist. Consider adding try-catch blocks and file existence checks.

-fs.copyFileSync('README.md', path.join('dist', 'README.md'))
-fs.copyFileSync('src/theme-tool.js', path.join('dist', 'theme-tool.js'))
-fs.copyFileSync('src/theme-tool.d.ts', path.join('dist', 'theme-tool.d.ts'))
+const filesToCopy = [
+  ['README.md', 'dist/README.md'],
+  ['src/theme-tool.js', 'dist/theme-tool.js'],
+  ['src/theme-tool.d.ts', 'dist/theme-tool.d.ts']
+];
+
+filesToCopy.forEach(([src, dest]) => {
+  try {
+    if (!fs.existsSync(src)) {
+      throw new Error(`Source file ${src} does not exist`);
+    }
+    fs.copyFileSync(src, dest);
+  } catch (error) {
+    console.error(`Failed to copy ${src} to ${dest}:`, error);
+    process.exit(1);
+  }
+});
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// 1、复制 theme-tool.js /README.md 到dist目录
fs.copyFileSync('README.md', path.join('dist', 'README.md'))
fs.copyFileSync('src/theme-tool.js', path.join('dist', 'theme-tool.js'))
fs.copyFileSync('src/theme-tool.d.ts', path.join('dist', 'theme-tool.d.ts'))
// 1、复制 theme-tool.js /README.md 到dist目录
const filesToCopy = [
['README.md', 'dist/README.md'],
['src/theme-tool.js', 'dist/theme-tool.js'],
['src/theme-tool.d.ts', 'dist/theme-tool.d.ts']
];
filesToCopy.forEach(([src, dest]) => {
try {
if (!fs.existsSync(src)) {
throw new Error(`Source file ${src} does not exist`);
}
fs.copyFileSync(src, dest);
} catch (error) {
console.error(`Failed to copy ${src} to ${dest}:`, error);
process.exit(1);
}
});


const root = path.resolve('./')
const content = await fs.promises.readFile(path.resolve(root, 'package.json'), 'utf8')
// 2、读取 old-theme-index.js , dist/old-theme-index.less， 合并后写入 dist/ old-theme-index.js
let jsStr = `
export default {
id: 'tiny-old-theme',
name: 'OldTheme',
cnName: '旧的主题',
css: \`#CSS#\`
}
`
let cssStr = fs.readFileSync(path.resolve(root, 'dist/old-theme-index.css'), 'utf8')

jsStr = jsStr.replace('#CSS#', cssStr)
fs.writeFileSync(path.resolve(root, 'src/old-theme-index.js'), jsStr) // 供开发时(pnpm site)， 可以访问到最新的定制主题变量
fs.writeFileSync(path.resolve(root, 'dist/old-theme-index.js'), jsStr)

// 3、复制 package.json
const content = fs.readFileSync(path.resolve(root, 'package.json'), 'utf8')
const packageJson = JSON.parse(content)
delete packageJson.exports
delete packageJson.private
await fs.promises.writeFile(path.resolve(root, 'dist/package.json'), JSON.stringify(packageJson, null, 2))
fs.writeFileSync(path.resolve(root, 'dist/package.json'), JSON.stringify(packageJson, null, 2))
Comment on lines +26 to +31
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add error handling for package.json operations.

While the static analysis flags the delete operator, its usage here is acceptable as this is a build script where performance is not critical, and we're modifying a parsed JSON object, not an object with prototypes.

However, the code should handle potential errors:

-const content = fs.readFileSync(path.resolve(root, 'package.json'), 'utf8')
-const packageJson = JSON.parse(content)
-delete packageJson.exports
-delete packageJson.private
-fs.writeFileSync(path.resolve(root, 'dist/package.json'), JSON.stringify(packageJson, null, 2))
+try {
+  const content = fs.readFileSync(path.resolve(root, 'package.json'), 'utf8')
+  const packageJson = JSON.parse(content)
+  delete packageJson.exports
+  delete packageJson.private
+  fs.writeFileSync(
+    path.resolve(root, 'dist/package.json'),
+    JSON.stringify(packageJson, null, 2)
+  )
+} catch (error) {
+  console.error('Failed to process package.json:', error)
+  process.exit(1)
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// 3、复制 package.json
const content = fs.readFileSync(path.resolve(root, 'package.json'), 'utf8')
const packageJson = JSON.parse(content)
delete packageJson.exports
delete packageJson.private
await fs.promises.writeFile(path.resolve(root, 'dist/package.json'), JSON.stringify(packageJson, null, 2))
fs.writeFileSync(path.resolve(root, 'dist/package.json'), JSON.stringify(packageJson, null, 2))
// 3、复制 package.json
try {
const content = fs.readFileSync(path.resolve(root, 'package.json'), 'utf8')
const packageJson = JSON.parse(content)
delete packageJson.exports
delete packageJson.private
fs.writeFileSync(
path.resolve(root, 'dist/package.json'),
JSON.stringify(packageJson, null, 2)
)
} catch (error) {
console.error('Failed to process package.json:', error)
process.exit(1)
}
🧰 Tools
🪛 Biome

[error] 31-31: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

[error] 31-31: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

4 changes: 2 additions & 2 deletions packages/theme/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@
},
"main": "index.css",
"scripts": {
"clean": "rimraf dist src/aurora-theme src/smb-theme",
"clean": "rimraf dist",
"build:theme": "gulp build --gulpfile build/gulp-dist.js",
"build": "npm run clean && npm run build:theme",
"release": "node build/release.js",
"build:fast": "npm run build && npm run release",
"release": "node build/release.js",
"build:copy-remote": "npm run build:theme && cp-cli dist ../tiny-vue/node_modules/@opentiny/vue-theme",
"publishTgz": "node .cloudbuild/publish-tgzs.js",
"postversion": "pnpm build",
Expand Down
1 change: 0 additions & 1 deletion packages/theme/src/base/index.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,3 @@
@import './reset.less';
@import './vars.less';
@import './transition.less';
@import '../svg/index.less';
2 changes: 1 addition & 1 deletion packages/theme/src/base/reset.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
*/

@import '../custom.less';
@import './basic-var.less';
@import './vars.less';

// .tiny-icon-loading 类名的动画效果。 它出现在多个组件中，故抽取到一起。
.@{css-prefix-iconfont}-loading {
Expand Down
2 changes: 1 addition & 1 deletion packages/theme/src/button/old-theme.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@

.@{button-prefix-cls} {
// 默认时按钮字重
--tv-Button-font-weight: var(--tv-font-weight-thin);
--tv-Button-border-radius: 6px;
}
1 change: 0 additions & 1 deletion packages/theme/src/index.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@
@import './statistic/index.less';
@import './steps/index.less';
@import './sticky/index.less';
@import './svg/index.less';
@import './switch/index.less';
@import './table/index.less';
@import './tabs/index.less';
Expand Down
6 changes: 6 additions & 0 deletions packages/theme/src/old-theme-index.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 0 additions & 2 deletions packages/theme/src/old-theme.less
View file
Open in desktop

This file was deleted.

37 changes: 27 additions & 10 deletions packages/theme/src/theme-tool.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,32 @@
interface ThemeData {
id: string
name: string
cnName: string
data: object
id?: string
name?: string
cnName?: string
/**
* 需要追加的全局css变量的对象。
* 比如： { 'tv-base-color-brand' : '#1476ff' } 会追加到 :root { --tv-base....... }
* */
data?: Record<string, string>
Comment on lines +5 to +9
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding type safety for CSS variable names.

While Record<string, string> is better than object, we can further improve type safety for CSS variables.

Consider creating a more specific type for CSS variable names:

type CSSVariableName = `--tv-${string}`

interface ThemeData {
  // ...
  data?: Record<CSSVariableName, string>
  // ...
}

This ensures that all variable names follow the --tv- prefix convention shown in the example.

/**
* 需要追加的样式规则, 以覆盖或扩充组件的样式。
* 比如： .tiny-button { border:none; }
* */
css?: string
}

/**
* 动态切换文档或影子根节点的样式类
* @example
* const themeTool= new TinyThemeTool();
* themeTool.changeTheme(xxx)
*/
export default class TinyThemeTool {
currentTheme: string
contentElement: HTMLElement
styleSheetId: string
defaultTheme: object
changeTheme: (theme: ThemeData) => void
constructor(theme: ThemeData, styleSheetId: string)
constructor(theme?: ThemeData)
/**
* 变更目标上的主题。
* 它会汇总 theme 下 data 和 css 属性的有效样式，挂载到target 节点下
* @param {ThemeData} theme - 变量的主题数据
* @param {Document | ShadowRoot} target - 变量的挂载点，不指定时默认为 document
*/
changeTheme: (theme: ThemeData, target?: Document | ShadowRoot) => void
}
41 changes: 40 additions & 1 deletion packages/theme/src/theme-tool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,40 @@
export default class TinyThemeTool {}
import OldTheme from './old-theme-index.js'

export { OldTheme }

/**
* 动态切换文档或影子根节点的样式类
*/
export default class TinyThemeTool {
constructor(theme) {
this.loaded = {} // 缓存已加载的样式
if (theme) {
this.changeTheme(theme)
}
}

changeTheme(theme, target = document) {
let loadedSheet = this.loaded[target]
if (!loadedSheet) {
loadedSheet = new CSSStyleSheet()
target.adoptedStyleSheets = [...target.adoptedStyleSheets, loadedSheet]
this.loaded[target] = loadedSheet
}

Comment on lines +16 to +22
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add browser compatibility check and fallback.

The adoptedStyleSheets API isn't supported in all browsers. Add a fallback mechanism using traditional stylesheet injection.

 changeTheme(theme, target = document) {
+  if (!('adoptedStyleSheets' in target)) {
+    // Fallback for browsers not supporting adoptedStyleSheets
+    let styleElement = target._tinyThemeStyle;
+    if (!styleElement) {
+      styleElement = document.createElement('style');
+      target._tinyThemeStyle = styleElement;
+      (target.head || target).appendChild(styleElement);
+    }
+    this.loaded[target] = styleElement;
+    return this._applyThemeClassic(theme, styleElement);
+  }
+
   let loadedSheet = this.loaded[target]
   if (!loadedSheet) {
     loadedSheet = new CSSStyleSheet()
     target.adoptedStyleSheets = [...target.adoptedStyleSheets, loadedSheet]
     this.loaded[target] = loadedSheet
   }

Committable suggestion was skipped due to low confidence.

if (theme && (theme.data || theme.css)) {
let cssContent = ''
if (theme.data && typeof theme.data === 'object') {
cssContent = Object.keys(theme.data)
.map((key) => `--${key}: ${theme.data[key]}; `)
.join('')

Comment on lines +27 to +29
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Sanitize theme data to prevent CSS injection vulnerabilities.

Currently, the keys and values from theme.data are directly inserted into CSS without sanitization. This could lead to CSS injection if theme.data contains malicious content. Consider using CSS.escape() to sanitize both the keys and values.

Apply this diff to sanitize the keys and values:

cssContent = Object.keys(theme.data)
-     .map((key) => `--${key}: ${theme.data[key]}; `)
+     .map((key) => `--${CSS.escape(key)}: ${CSS.escape(theme.data[key])}; `)
      .join('');
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
cssContent = Object.keys(theme.data)
.map((key) => `--${key}: ${theme.data[key]}; `)
.join('')
cssContent = Object.keys(theme.data)
.map((key) => `--${CSS.escape(key)}: ${CSS.escape(theme.data[key])}; `)
.join('')

cssContent = `:root{${cssContent}}`
}
if (theme.css && typeof theme.css === 'string') {
cssContent += theme.css
}

Comment on lines +33 to +35
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure theme.css is from a trusted source to prevent CSS injection.

Appending theme.css directly to cssContent could introduce security risks if theme.css contains malicious code. Ensure that theme.css is from a trusted source or implement sanitization to prevent CSS injection attacks.

loadedSheet.replaceSync(cssContent)
}
}
}
Comment on lines +16 to +39
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add type validation for theme object.

The method should validate the theme object structure to prevent runtime errors and improve debugging.

+/**
+ * @typedef {Object} Theme
+ * @property {Record<string, string|number>} [data] - Theme CSS variables
+ * @property {string} [css] - Raw CSS content
+ */
+
+/**
+ * Changes the theme for a target
+ * @param {Theme} theme - The theme configuration
+ * @param {Document|ShadowRoot} target - The target to apply theme to
+ * @throws {Error} If theme object is invalid
+ */
 changeTheme(theme, target = document) {
+  if (theme && typeof theme !== 'object') {
+    throw new TypeError('Theme must be an object');
+  }
+  if (theme?.data && typeof theme.data !== 'object') {
+    throw new TypeError('Theme.data must be an object');
+  }
+  if (theme?.css && typeof theme.css !== 'string') {
+    throw new TypeError('Theme.css must be a string');
+  }

Committable suggestion was skipped due to low confidence.

Loading