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

feat: check mf.name is valid identifier #10428

Merged
merged 6 commits into from
Feb 7, 2023

Conversation

AkaraChen
Copy link
Member

close #10424

@vercel
Copy link

vercel bot commented Feb 6, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
umi ⬜️ Ignored (Inspect) Feb 6, 2023 at 7:18AM (UTC)

@codecov
Copy link

codecov bot commented Feb 6, 2023

Codecov Report

Base: 29.29% // Head: 29.25% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (ecc4c6a) compared to base (3c78dbe).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10428      +/-   ##
==========================================
- Coverage   29.29%   29.25%   -0.05%     
==========================================
  Files         445      445              
  Lines       13006    13048      +42     
  Branches     3178     3188      +10     
==========================================
+ Hits         3810     3817       +7     
- Misses       8592     8621      +29     
- Partials      604      610       +6     
Impacted Files Coverage Δ
packages/plugins/src/mf.ts 55.35% <0.00%> (-4.26%) ⬇️
packages/plugin-run/src/index.ts 0.00% <0.00%> (ø)
packages/lint/src/config/eslint/index.ts 0.00% <0.00%> (ø)
packages/preset-umi/src/commands/verify-commit.ts 0.00% <0.00%> (ø)
...ackages/preset-umi/src/features/prepare/prepare.ts 0.00% <0.00%> (ø)
...kages/preset-umi/src/features/apiRoute/apiRoute.ts 0.00% <0.00%> (ø)
...kages/preset-umi/src/features/monorepo/redirect.ts 0.00% <0.00%> (ø)
...kages/preset-umi/src/features/polyfill/polyfill.ts 0.00% <0.00%> (ø)
...kages/preset-umi/src/features/tmpFiles/tmpFiles.ts 0.00% <0.00%> (ø)
...ages/bundler-webpack/src/config/javaScriptRules.ts 74.28% <0.00%> (ø)
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@AkaraChen AkaraChen marked this pull request as draft February 6, 2023 06:02
@fz6m
Copy link
Contributor

fz6m commented Feb 6, 2023

把所有 name 聚合一下,跑一下 esbuild ,如果报错了,告诉是第几行的 name 有啥问题,demo :

import esbuild from 'esbuild'

try {

  const r = esbuild.transformSync(
    `
  export const a-1 = 1;
  export const default  = 1;
  `,
    {
      target: 'es2015',
      format: 'esm',
    }
  )
  
  console.log('r: ', r)

catch (e) {
  // logger ...
  throw new Error('mfname must be a valid JavaScript variable declaration', { cause: e }) // 有问题直接报错
}

@@ -263,4 +270,76 @@ export default function mf(api: IApi) {
function addMFEntry(config: any, mfName: string, path: string) {
config.entry[mfName] = path;
}

function isValidIdentifyName(name: string) {
const identifierList = [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const identifierList = [
const reservedKeywords = [

];
const regexIdentifierName =
/^(?:[$_\p{ID_Start}])(?:[$_\u200C\u200D\p{ID_Continue}])*$/u;
if (identifierList.includes(name) || !regexIdentifierName.test(name)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (identifierList.includes(name) || !regexIdentifierName.test(name)) {
if (reservedKeywords.includes(name) || !regexIdentifierName.test(name)) {

Co-Authored-By: pshu <415655+stormslowly@users.noreply.github.com>
@@ -226,6 +226,13 @@ export default function mf(api: IApi) {
);
}

if (!isValidIdentifyName(name)) {
api.logger.warn(
Copy link
Member

Choose a reason for hiding this comment

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

我在想 如果用户主动配置 但是配置错误了, 说明他是想用这个功能的

这个时候直接抛出异常,中断启动会不会更好。

ps
unNameMF 这个也是我当时没有想清楚做的默认值,如果启动了插件,并且有导出的模块,但是没有命名,我觉得也应该抛异常,这个时候说明用户使用错误;如果继续这么用下去可能会出现两个 unNamedMF 的命名冲突;或再提供下游使用的时候才发现这个问题。
这里先记录下,这个可以在另外一个 PR 搞

@stormslowly
Copy link
Member

把所有 name 聚合一下,跑一下 esbuild ,如果报错了,告诉是第几行的 name 有啥问题,demo :

import esbuild from 'esbuild'

try {

  const r = esbuild.transformSync(
    `
  export const a-1 = 1;
  export const default  = 1;
  `,
    {
      target: 'es2015',
      format: 'esm',
    }
  )
  
  console.log('r: ', r)

catch (e) {
  // logger ...
  throw new Error('mfname must be a valid JavaScript variable declaration', { cause: e }) // 有问题直接报错
}

只是验证名字语法上是否合法就够了,没有必要用到编译工具;感觉这样太重了。

@AkaraChen
Copy link
Member Author

把所有 name 聚合一下,跑一下 esbuild ,如果报错了,告诉是第几行的 name 有啥问题,demo :

import esbuild from 'esbuild'

try {

  const r = esbuild.transformSync(
    `
  export const a-1 = 1;
  export const default  = 1;
  `,
    {
      target: 'es2015',
      format: 'esm',
    }
  )
  
  console.log('r: ', r)

catch (e) {
  // logger ...
  throw new Error('mfname must be a valid JavaScript variable declaration', { cause: e }) // 有问题直接报错
}

这块我也思考了怎么利用构建工具,但是还是感觉有点太重了,而且我也不太熟悉,就没用

@fz6m
Copy link
Contributor

fz6m commented Feb 6, 2023

esbuild 很快的,一瞬间就能全部检测完,应该往二进制工具上靠,正则的话,要维护大量不确定的 keyword ,还有 edge case ,性能还差。

@AkaraChen AkaraChen marked this pull request as ready for review February 6, 2023 06:46
@AkaraChen
Copy link
Member Author

esbuild 很快的,一瞬间就能全部检测完,应该往二进制工具上靠,正则的话,要维护大量不确定的 keyword ,还有 edge case ,性能还差。

这个正则是我 copy from tc39/proposal-regexp-unicode-property-escapes 的,我也是感觉靠谱才用这个方案的,否则我也不会用正则的

@stormslowly
Copy link
Member

esbuild 很快的,一瞬间就能全部检测完,应该往二进制工具上靠,正则的话,要维护大量不确定的 keyword ,还有 edge case ,性能还差。

我得理解 esbuild 会走一次进程间通信,盲猜速度上可能正则会快一点。
keyword 是语法定义好了,如果 TC39 说要加 keyword esbuild 也得升级。

@stormslowly
Copy link
Member

esbuild 很快的,一瞬间就能全部检测完,应该往二进制工具上靠,正则的话,要维护大量不确定的 keyword ,还有 edge case ,性能还差。

这个正则是我 copy from tc39/proposal-regexp-unicode-property-escapes 的,我也是感觉靠谱才用这个方案的,否则我也不会用正则的

那个正则上面,加个注释贴上链接吧(说实话我第一眼也没懂个正则。。。

@stormslowly
Copy link
Member

LGTM

@sorrycc sorrycc merged commit a3fb281 into umijs:master Feb 7, 2023
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.

[Feature Request] mf 插件的检查 name 是否为合法的变量名
4 participants