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

fix(core): fix type check for Date prop #12272

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Conversation

kikuchan
Copy link

@kikuchan kikuchan commented Sep 11, 2021

It checks a prop by [object Date] instead of instanceof Date

This fixes a strange warning message:
[Vue warn]: Invalid prop: type check failed for prop "date". Expected Date, got Date
in some circumstances.

This happens when type: Date's Date is in another context, such as used in dev mode in nuxt for example.

I've written and attached a unit test script for nodejs to reproduce this issue easily, and I've confirmed by using the script that this PR resolves the issue.
But I'm sorry I don't know how to write and include a unit test file suitable for this project, so please someone translate the test script suitable for the project.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:
This also happens on Vue3 and should be fixed in the same way.

related issues in other projects:
nuxt/nuxt#5565
buefy/nuxt-buefy#36

@posva
Copy link
Member

posva commented Sep 12, 2021

Do you have a repro? don't attach files, post plain code only

@kikuchan

This comment has been minimized.

@kikuchan
Copy link
Author

@posva I've made a much simpler code to reproduce, and now I know how to write a test code. Is this suitable?

@posva
Copy link
Member

posva commented Sep 12, 2021

Do you have an actual boiled down reproduction? Like on a js fiddle ?

@kikuchan
Copy link
Author

It's based on buefy/nuxt-buefy#36 (comment) but upgrade to the latest version of them.
https://codesandbox.io/s/codesandbox-nuxt-forked-kdlsy

I think nuxt and nuxt-buefy setup is the easiest way to reproduce.
I have no idea that why this combination triggers this issue, but I'm sure that multiple contexts are involved during the nuxt SSR process.
For Nuxt, according to the nuxt/nuxt#5565 (comment) , the behavior can be disabled by setting runInNewContext: false in the config file, but it's a workaround.

I've also checked value directly on

valid = value instanceof type;
by editing the file directly to inject console.log and/or to start REPL session in the context.
After some investigations, the value is surely an instance of v8's internal Date but value instanceof Date returns false there, because a Date a constructor of value, and a Date there, are in a different context.

@posva
Copy link
Member

posva commented Sep 12, 2021

That looks more like a big they should be fixing. Changing the global Date is not something tou should do

@kikuchan
Copy link
Author

Node has legitimate APIs to create a new running environment (context) for sandboxing or something else if you want.
Changing the global Date variable by the user is usually a bad idea and should be prohibited strictly, but I think this is not a case because it is done automatically/internally/implicitly by Node/v8 itself.

Although the two Dates are valid native Date constructors for Node/v8, Vue type checking relies on the current global constructor to identify whether it is an instance of Date or not, thus it fails.

By the way, I've also come up with a reproduction code to prove there is a case that Dates have different contexts each other in normal browser environment.

<html>
  <head>
    <script src="https://cdn.jsdelivr.net/npm/vue@2.6.14/dist/vue.js"></script>
  </head>
  <body>
    <div id="app">
      <div>{{ date }}</div>
      <test-component :date="date" />
    </div>

    <iframe src="child.html"></iframe>

    <script>
      const iframe = document.querySelector('iframe');
      iframe.onload = function () {
        const TestComponent = {
          template: '<div>{{ date }}</div>',
          props: {
            date: Date,
          }
        }

        new Vue({
          el: '#app',
          components: { TestComponent },

          data() {
            return {
              // XXX: Date instance comes from another context!
              date: iframe.contentWindow.date
            }
          },
        })
      };
    </script>
  </body>
</html>
<html>
  <head />
  <body>
    This is a child.
    <script>
      var date = new Date();
    </script>
  </body>
</html>

As you can see, I exploit <iframe> to make a specially crafted Date instance in this case.

Well... How about using the following code to fix the issue?
value instanceof Date || Object.prototype.toString.call(value) === '[object Date]'
In this way, I think it can minimize the chance of unexpected changes of behavior and can maximize backward compatibility.

@AnnikaCodes
Copy link

What's the status on this? I'm currently working on a Vue app with Nuxt and getting lots of the Expected Date, got Date errors. It's easy enough to ignore them but this is still a fix that would be useful (and make things way simpler for developers who are new to Vue + Nuxt).

@existe-deja
Copy link

A temporary fix is to clone the date object before transmitting to the child component with buggyDate = new Date(buggyDate.getTime())

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants