-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix: correct entry script identification and webpack version detectio… #2800
Conversation
🦋 Changeset detectedLatest commit: b40dba4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@nayonglin is attempting to deploy a commit to a Personal Account owned by @umijs on Vercel. @umijs first needs to authorize it. |
… into fix/webpack_plugin_opt
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.
如果用户已经指定了 entry 就不需要处理了,这块好像漏了?
已加上 |
@@ -20,6 +20,6 @@ describe('QiankunPlugin', () => { | |||
expect(htmlContent).toContain('<script entry'); // 检查是否正确标记了 entry js | |||
|
|||
const jsChunkContent = fs.readFileSync(path.join(__dirname, 'tests/webpack5/dist/bundle.js'), 'utf-8'); | |||
expect(jsChunkContent).toContain('window.'); // 检查是否包含了 window. 关键字 | |||
expect(jsChunkContent).toMatch(/window(\["[^"]+"\]|\.\w+)/); // 检查是否包含了 window. 或 window["..."] 关键字 |
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.
这里直接检查 window.${packageName} 吧
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.
这里直接检查 window.${packageName} 吧
已加上包名,不过同时判断了.符号和中括号的形式,因为发现在某些场景下webpack产物是中括号的这种形式
packages/webpack-plugin/src/index.ts
Outdated
let alreadyHasEntry = false; | ||
|
||
// 检查是否已经有带有entry属性的script标签 | ||
scriptTags.forEach((tag) => { |
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.
这里换成 scriptTags.some 更好
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.
如果 html 里找到了多个符合 entrySrcPattern 的 script,应该报错提示用户填的正则可能有问题
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.
如果 html 里找到了多个符合 entrySrcPattern 的 script,应该报错提示用户填的正则可能有问题
这部分已用cheerio重写
packages/webpack-plugin/src/index.ts
Outdated
private static packageJson: PackageJson = QiankunPlugin.readPackageJson(); | ||
|
||
constructor(options: QiankunPluginOptions = {}) { | ||
this.packageName = options.packageName || QiankunPlugin.packageJson.name || ''; | ||
this.entrySrcPattern = options.entrySrcPattern | ||
? new RegExp(`<script[^>]*src="[^"]*${options.entrySrcPattern.source}[^"]*"[^>]*></script>`, 'g') |
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.
用正则去匹配 script 很难写出没 bug 的版本,比如 script 这里有换行了
<script
src=""
>
更极端的 html 里有一个 script 的注释。
直接引一个 cheerio 来做 html 匹配的事情吧,可以直接用 css selector 做元素查找,会少很多 bug
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.
用正则去匹配 script 很难写出没 bug 的版本,比如 script 这里有换行了
<script src="" >更极端的 html 里有一个 script 的注释。
直接引一个 cheerio 来做 html 匹配的事情吧,可以直接用 css selector 做元素查找,会少很多 bug
用cheerio可以,不过用户需要传入什么会比较简单一点?还需要传入正则吗?
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.
好像没有比正则更简单的吧?
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.
好像没有比正则更简单的吧?
done
@nayonglin 这周能处理一下吗?准备发 3.0 正式版了😆 |
嗯嗯,我明天处理一下 |
🙋♂️这块可以到 cli 测试了吗? |
packages/webpack-plugin/src/index.ts
Outdated
} | ||
return htmlString; | ||
|
||
return $.html(); | ||
} | ||
} | ||
|
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.
这里还是改成 export class QiankunPlugin
使用 const { QiankunPlugin } = require('@qiankunjs/webpack-plugin')
用module.exports = QiankunPlugin
获取不到类型
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.
review
定位的行号有问题
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.
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.
先 approve 了,代码细节后面单独 pr 调整吧
if (!alreadyHasEntry) { | ||
if (!this.entrySrcPattern) { | ||
// 如果没有提供正则表达式,则选择最后一个 script 标签 | ||
$('script').last().attr('entry', ''); |
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.
需要是最后一个外联 script,inline 的不支持
$('script').last().attr('entry', ''); | ||
} else { | ||
// 使用提供的正则表达式过滤 script 标签 | ||
const matchingScriptTags = $('script').filter((_, el) => { |
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.
这里可以直接取 srcipt[src] 的标签,不用 filter 里再过滤一遍
Checklist
npm test
passesDescription of change