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

Order of plugin extends breaks plugin functions #2027

Closed
Worly opened this issue Aug 17, 2022 · 6 comments
Closed

Order of plugin extends breaks plugin functions #2027

Worly opened this issue Aug 17, 2022 · 6 comments

Comments

@Worly
Copy link

Worly commented Aug 17, 2022

Describe the bug
I'm trying to use two plugins (duration and objectSupport) in my Angular app. While using datejs#add function with argument of type Duration (implemented in duration plugin), I get Invalid Date if the plugin objectSupport is extended after the duration plugin. If objectSupport is extended before the duration plugin then add method works fine.

Demo
CodeSandbox link https://codesandbox.io/s/festive-sutherland-thxuux?file=/src/app/app.module.ts
In app.module.ts, change the order of lines 10 and 11, and look at console after refreshing Browser preview tab.

Expected behavior
No matter the order of plugin extends, all plugin functions should work as expected.

Information

  • Day.js Version ^1.11.5
  • OS: Windows 10
  • Browser Chrome/103.0.5060.134
@BePo65
Copy link
Contributor

BePo65 commented Aug 18, 2022

As a basic pattern, plugins create a chain of function calls for overwritten / extended functions (like parse or add).
Therefore the sequence of extending dayjs is important (and should be documented in the documentation site).

Some plugins depend on other plugins. In that case this is part of the documentation and means that the dependant plugin must be extend-ed after the prerequisite plugin.

duration and objectSupport are a special case: the problem is that a Duration object is instanceof Duration and of course also instanceof Object and that information is used e.g. in parsing. So the duration plugin must be extend-ded after the objectSupport plugin and that information is missing in the documentation.

Perhaps you want to create a PR in the docu repo?

@iamkun
Copy link
Owner

iamkun commented Aug 18, 2022

Maybe we can update the logic something like this

class Duration {
  constructor() {
    this.isDurationObj = true 
  }
}
// then
argument instanceof Object && !argument.isDurationObj

to fix this issue?


btw, @BePo65 , do you mind taking a look at a minor update in the comments in #1914 so that we can get it merged?

@BePo65
Copy link
Contributor

BePo65 commented Aug 19, 2022

I am afraid that this will not work, as the class Duration is not known in the objectSupport plugin (or anywhere outside of the duration plugin).

We could add something like this to the start of the initialization of the objectSupport plugin:

  if (dayjs.duration !== 'undefined') {
    console.error('the plugin objectSupport should be extended before the plugin Duration')
  }

but IMHO two plugins should not directly reference each other. And wouldn't we have also do this for plugins that depend on other plugins?

Although nobody reads the documentation 😄 I would simply update the documentation (where this consequence is not documented until today).

BTW: there a few pr for the documentation website that are perhaps worth a little bit of your time - shouldn't be too much work to review them 😃

@iamkun
Copy link
Owner

iamkun commented Aug 22, 2022

It could be possible.

cause in ObjectSupport plugin, it will check if the argument is an object, argument instanceof Object

all we have to do, is to add a property to the passed object in Duration plugin so that the logic !argument.isDurationObj will know if it is a duration object.

BePo65 added a commit to BePo65/dayjs that referenced this issue Aug 24, 2022
while fixing, an issue wit the handling of subtract appeared and was
fixed too.
BePo65 added a commit to BePo65/dayjs that referenced this issue Aug 24, 2022
BePo65 added a commit to BePo65/dayjs that referenced this issue Aug 24, 2022
while fixing, an issue wit the handling of subtract appeared and was
fixed too.
BePo65 added a commit to BePo65/dayjs that referenced this issue Aug 24, 2022
@BePo65
Copy link
Contributor

BePo65 commented Aug 24, 2022

Of course it would be possible, but IMO objectSupport should not know anything about other plugins it does not depend on.

I think I found a solution that does respect this requirement.
If we use obj.constructor.name === 'Object' instead of obj instanceof Object we can differentiate between plain objects and objects that are instances of a class. And as 'Object' is a native type, it will not get a new name, when the code is minified which would happen with class names.

I created pr #2038 for this.
While I was on it, I found and fixed a hidden problem in objectSupport (the subtract method was based on the fact that subtracting is adding a negative value - which is true for numbers only).

iamkun pushed a commit that referenced this issue Aug 30, 2022
…2038)

* fix: make objectSupport ignore other object types (issue #2027)

while fixing, an issue wit the handling of subtract appeared and was
fixed too.

* test: add tests for issue #2027
@iamkun
Copy link
Owner

iamkun commented Aug 30, 2022

fixed via #2038

@iamkun iamkun closed this as completed Aug 30, 2022
iamkun pushed a commit that referenced this issue Oct 21, 2022
## [1.11.6](v1.11.5...v1.11.6) (2022-10-21)

### Bug Fixes

* add BigIntSupport plugin ([#2087](#2087)) ([f6dce48](f6dce48))
* Fix objectSupport collides with Duration plugin - issue [#2027](#2027) ([#2038](#2038)) ([c9370ea](c9370ea))
ohsory1324 pushed a commit to ohsory1324/dayjs that referenced this issue Dec 20, 2023
## [1.11.6](iamkun/dayjs@v1.11.5...v1.11.6) (2022-10-21)

### Bug Fixes

* add BigIntSupport plugin ([iamkun#2087](iamkun#2087)) ([f6dce48](iamkun@f6dce48))
* Fix objectSupport collides with Duration plugin - issue [iamkun#2027](iamkun#2027) ([iamkun#2038](iamkun#2038)) ([c9370ea](iamkun@c9370ea))
splashwizard pushed a commit to splashwizard/tracking-time that referenced this issue Oct 21, 2024
## [1.11.6](iamkun/dayjs@v1.11.5...v1.11.6) (2022-10-21)

### Bug Fixes

* add BigIntSupport plugin ([#2087](iamkun/dayjs#2087)) ([f6dce48](iamkun/dayjs@f6dce48))
* Fix objectSupport collides with Duration plugin - issue [#2027](iamkun/dayjs#2027) ([#2038](iamkun/dayjs#2038)) ([c9370ea](iamkun/dayjs@c9370ea))
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

3 participants