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.

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

build: embedding icons #132

Closed
wants to merge 8 commits into from
Closed

Conversation

FoundTheWOUT
Copy link
Contributor

close #108
使用 vite-plugin-svgr 后就不用担心这个问题了。但打包文件中还是残留了多余的文件

之前

Snipaste_2022-11-13_22-30-28

现在

Snipaste_2022-11-13_22-29-54

@trim21
Copy link
Contributor

trim21 commented Nov 13, 2022

vite就不能像webpack一样直接加个loader了事吗,还要自己写资源文件到js的转换(

@github-actions
Copy link
Contributor

preview url: https://pr-132--bangumi-next.netlify.app

@github-actions
Copy link
Contributor

storybook preview url: https://pr-132-storybook--bangumi-next.netlify.app

@FoundTheWOUT
Copy link
Contributor Author

FoundTheWOUT commented Nov 14, 2022

不知道为啥单元测试没过
没理解为什么之前的 icon 会渲染成 div
@cokemine 能帮忙看看这是什么情况吗

@trim21 trim21 changed the title refactor: build icon build: embedding icons Nov 14, 2022
Copy link
Contributor

@Ayase-252 Ayase-252 left a comment

Choose a reason for hiding this comment

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

为啥会多两个 SVG?

@FoundTheWOUT
Copy link
Contributor Author

为啥会多两个 SVG?

是指两个箭头吗,我只是改了一下 fill,之前没 fill,导致 icon 默认透明

@@ -5,12 +5,12 @@
"scripts": {
"prepare": "husky install",
"preinstall": "npx only-allow pnpm",
"build": "pnpm website build",
"build": "pnpm icons build && pnpm website build",
Copy link
Contributor

Choose a reason for hiding this comment

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

感觉build越来越复杂了...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

还好吧,才两个build(
可以写下script,或者上turborepo

@github-actions
Copy link
Contributor

github-actions bot commented Nov 18, 2022

// export all svg
await writeFile(join(OUT_DIR, 'index.js'), indexContent, { encoding: 'utf-8' });
await writeFile(join(OUT_DIR, 'index.d.ts'), indexContent, { encoding: 'utf-8' });
} catch (error) {
Copy link
Contributor

@trim21 trim21 Nov 18, 2022

Choose a reason for hiding this comment

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

这里不用catch吧,catch了之后构建失败只有一个console.error了,依旧会去构建后面的website

@trim21
Copy link
Contributor

trim21 commented Nov 18, 2022

看了一下,是不是直接用这个就好了

vitejs/vite#1204 (comment)

@FoundTheWOUT
Copy link
Contributor Author

FoundTheWOUT commented Nov 18, 2022

看了一下,是不是直接用这个就好了

vitejs/vite#1204 (comment)

其实现在已经是用了 vite-plugin-svgr 去做了 embedding 的工作了😂,我这个 PR 里把 svg 打包一次是防止 vite 直接处理 svg(就是把 svg 当成静态文件复制到 dist)。
所以一开始我的标题是 build icons,没写 embedding icons

@trim21
Copy link
Contributor

trim21 commented Nov 18, 2022

那我理解错了... 所以原本就不会请求一堆小svg,只是单纯的有多于文件而已?

@FoundTheWOUT
Copy link
Contributor Author

那我理解错了... 所以原本就不会请求一堆小svg,只是单纯的有多于文件而已?

是的

@trim21
Copy link
Contributor

trim21 commented Nov 18, 2022

是的

但是我感觉issue108不是这个意思?如果只是在build里有多余文件的话无所谓了。

@FoundTheWOUT
Copy link
Contributor Author

FoundTheWOUT commented Nov 18, 2022

但是我感觉issue108不是这个意思?如果只是在build里有多余文件的话无所谓了。

实话说。。确实无所谓(我本身的目的也只是减小打包体积)
108 的话,可能我理解错了,我以为108里指 svg 太多,需要发起多个请求而影响加载效果。

@trim21
Copy link
Contributor

trim21 commented Nov 18, 2022

指 svg 太多,需要发起多个请求而影响加载效果。

我也是这么理解的

@FoundTheWOUT
Copy link
Contributor Author

🤔所以其实我只是想顺带 close 一下108 而已,可能我在这个 PR 里 ref 了那个 issue 导致了一些误解(

@trim21
Copy link
Contributor

trim21 commented Nov 18, 2022

既然这样的话那就不合了

@trim21 trim21 closed this Nov 18, 2022
@trim21 trim21 mentioned this pull request Nov 18, 2022
@FoundTheWOUT FoundTheWOUT deleted the icons branch January 9, 2023 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build: svg icon 雪碧图
3 participants