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 5 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.

9 changes: 4 additions & 5 deletions packages/theme/src/theme-tool.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,13 @@ interface ThemeData {
id: string
name: string
cnName: string
/** 全局css变量覆盖的对象，比如： { 'tv-base-color-brand' : '#1476ff' }*/
data: object
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 using a more specific type for the data property.

The generic object type provides little type safety. Since this is for CSS variable overrides, consider using a more specific type definition:

- data: object
+ data: Record<string, string>

This change would:

  • Ensure all keys and values are strings
  • Provide better type checking for CSS variable names and values
  • Enable better IDE autocompletion
📝 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
/** 全局css变量覆盖的对象，比如： { 'tv-base-color-brand' : '#1476ff' }*/
data: object
/** 全局css变量覆盖的对象，比如： { 'tv-base-color-brand' : '#1476ff' }*/
data: Record<string, string>

/** 需要追加的样式表 */
css: string
}

export default class TinyThemeTool {
currentTheme: string
contentElement: HTMLElement
styleSheetId: string
defaultTheme: object
changeTheme: (theme: ThemeData) => void
constructor(theme: ThemeData, styleSheetId: string)
constructor(theme: ThemeData)
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the gathered information, I can now generate the final response as there are clear issues that need to be addressed.

Constructor parameter is required but not being provided

The new TinyThemeTool class requires a theme parameter in its constructor, but the current usage in the codebase doesn't provide it:

  • examples/docs/newsrc/uses/useTheme.js: const theme = new TinyThemeTool()
  • examples/sites/src/tools/useTheme.js: const theme = new TinyThemeTool()

This will cause the constructor to fall back to the default theme, but it's better to explicitly provide the initial theme. Additionally, the implementation in packages/theme-saas/src/theme-tool.js shows that the class still internally uses all the removed properties (styleSheetId, currentTheme, contentElement, defaultTheme), suggesting a mismatch between the type definition and the actual implementation.

Required changes:

  • Update TinyThemeTool instantiation calls to provide the required theme parameter
  • Align the implementation with the type definition by properly encapsulating the internal properties
🔗 Analysis chain

LGTM! Verify theme-related functionality remains intact.

The simplified class interface with better encapsulation looks good. The removal of implementation details from the public interface and the addition of the changeTheme method provides a cleaner API.

Let's verify that all theme-related functionality is properly handled:

Let's gather more context about the usage patterns to ensure we have a complete picture.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any direct references to removed properties that might break
rg -l "styleSheetId|currentTheme|contentElement|defaultTheme" --type ts --type js

# Check for proper theme initialization patterns
ast-grep --pattern 'new TinyThemeTool($$$)'

Length of output: 604

Script:

#!/bin/bash
# Check the content of files that initialize TinyThemeTool to see if they're passing required theme parameter
rg -A 5 -B 5 "new TinyThemeTool" examples/docs/newsrc/uses/useTheme.js examples/sites/src/tools/useTheme.js

# Check how the removed properties are being used in the files
rg "styleSheetId|currentTheme|contentElement|defaultTheme" --type ts --type js -C 3

Length of output: 14595

}
38 changes: 37 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,37 @@
export default class TinyThemeTool {}
import OldTheme from './old-theme-index.js'

export { OldTheme }

export default class TinyThemeTool {
constructor(theme) {
if (theme) {
this.changeTheme(theme)
}
}

changeTheme(theme) {
if (this._loadedSheet) {
document.adoptedStyleSheets = document.adoptedStyleSheets.filter((t) => t !== this._loadedSheet)
}

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 handling browsers that do not support adoptedStyleSheets.

The use of document.adoptedStyleSheets is not supported in all browsers, such as Internet Explorer and some versions of Safari. To enhance compatibility, consider adding a fallback for browsers that do not support this API.

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.

const sheet = new CSSStyleSheet()
sheet.replaceSync(cssContent)

document.adoptedStyleSheets = [...document.adoptedStyleSheets, sheet]
this._loadedSheet = sheet
}
}
}
Loading