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

dayjs(undefined).isValid() show devHelper warn in development #1240

Open
lqzhgood opened this issue Nov 26, 2020 · 4 comments
Open

dayjs(undefined).isValid() show devHelper warn in development #1240

lqzhgood opened this issue Nov 26, 2020 · 4 comments

Comments

@lqzhgood
Copy link

lqzhgood commented Nov 26, 2020

Describe the bug
This is not a bug

Expected behavior

// English

I think the motivation for executing the isvalid() function is to verify that the input parameter value is valid. Because it makes no sense to execute dayjs().isvalid() in the production environment.
When the input parameter is undefined, it is more likely that the input parameter is undefined rather than the value is not passed.

Since there is no way to tell whether the input parameter is undefined or not passed in, I recommend that you provide a warning in development mode.

// Chinese

我认为执行 isValid() 函数时的动机是验证入参值是否为有效。因为在生产环境执行 dayjs().isvalid() 是没有意义的。
当入参为 undefined 时多半是因为入参为 undefined,而非未传值。

因为无法区分入参为undefined还是入参未传入,因此我建议在开发模式提供警告。


Information

  • Day.js Version 1.9.6
  • OS: Windows
  • Browser Chrome 86
  • Time zone: UTC+08:00
@lqzhgood
Copy link
Author

I don't know how to get cfg.args in this by Dayjs.isValid, can't PR~

@iamkun
Copy link
Owner

iamkun commented Nov 26, 2020

Thanks, it's a great idea to add some warning to DelHelper for a better development experience.

However, in this case, dayjs() is a common API to get the current date time https://day.js.org/docs/en/parse/now. It seems hard to tell if our user needs the current time or just passed an undefined by error.

@lqzhgood
Copy link
Author

lqzhgood commented Nov 27, 2020

English

I think the key point isdayjs ()and dayjs (undefined) in fact, there is a difference between, but dayjs its equivalent (https://dayjs.gitee.io/docs/en/parse/now).
dayjs(undefined) has a high probability of inclusion error rather than deliberate action.

 The exception is' dayjs(undefined, {config: XXX}) ', which gets the current time based on the configuration or plug-in.

Chinese

我认为这个关键点是 dayjs() 和 dayjs(undefined) 其实是有区别的,但是 dayjs 将其等同(https://dayjs.gitee.io/docs/en/parse/now)。
dayjs(undefined) 大概率是入参错误而非故意为之。

 例外是  `dayjs(undefined , { config:xxx } )` 根据配置或者插件获取当前时间。 

cfg.args = arguments// eslint-disable-line prefer-rest-params

function abc(a) {
    console.log('arguments', arguments);
}

const a = abc();  //    ----> {}
const b = abc(1); //   ----> { '0': 1 }
const c = abc(undefined);  // ----> { '0': undefined }

// a[0] --> undefined 
// c[0] --> undefined 

Object.keys(a) // ---> []
Object.keys(c) // ---> ["0"]

English

Apart from the above reasons, the execution .isValid () 'in the production environment logically means that the high probability is expected to verify whether the input parameter is correct, so the input parameter should exist, then dayjs(undefined).isValid() should return false

However, the high probability of executing dayj().isValid() in a production environment is meaningless and such code should almost never occur, even if the error warning is provided in the developer mode, which I think is more beneficial than harmful.

Now true is returned because dayjs() is equivalent to dayj(undefined), resulting in dayj(undefined).isValid() being equivalent to dayj().isValid()

At the very least, a warning should be provided in development mode when the isValid function is executed and the argument is undefined .

Chinese

抛开以上的原因,在生产环境中执行 .isValid() 从逻辑是来说大概率是希望验证入参是否正确,那么入参应该是存在的,那么dayjs(undefined).isValid()应该返回 false

而在生产环境中执行 dayj().isValid() 大概率是无意义的,这样的代码应该几乎不会出现,即便在开发者模式中给出 错误 的警告,我认为是利大于弊的。

现在因为 dayjs()dayj(undefined) 等同,导致 dayj(undefined).isvalid() 等同于 dayj().isValid() 而返回 true

那么至少应该在执行 isValid 函数且入参为 undefined 时在开发模式提供警告.

// add code after https://github.com/iamkun/dayjs/blob/eb5fbc4c7d1b62a8615d2f263b404a9515d8e15c/src/plugin/devHelper/index.js#L16
   const oldIsValid = proto.isValid;
   proto.isValid= function () {
     const cfg = this.xxxx;  // <---- can't found `cfg` in Dayjs Class
     const args = cfg.args;   
     if (Object.keys(args).length > 0 && args[0] === undefined ){
       console.warn(`To parse a date-time string is undefined,  dayjs(undefined) === dayjs()  . https://day.js.org/docs/en/parse/now`);
     }
     return oldIsValid.bind(this)()
   }

@iamkun
Copy link
Owner

iamkun commented Nov 30, 2020

Nice catch. It's good to add a dev warn while passing undefined to the constructor.

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

No branches or pull requests

2 participants