fix: remove symbol props from stubs #1086

lmiller1990 · 2021-11-18T00:05:16Z

resolves #1076

More context: ionic-team/ionic-framework#24181

Basically when you do a stub that receives a Symbol as a prop, it ends up doing <blah-stub sym="Symbol()"> which isn't allowed. Try it in your browser: document.body.setAttribute('sym', Symbol()).

I just filter out all the symbol props. Bit of a hack, but not a breaking change (passing a symbol as prop to a stub causes an error in both VTU v1 and v2).

Happy to solicit any better work arounds for this issue, best I could come up with for now to unblock the issue in Ionic team repo.

cexbrayat

This makes me think that the fix should not be in VTU but in Vue itself (as it won't be in webidl-conversion).
The same will happen in Vue if someone binds a Symbol to an attribute right?
So instead of having this dirty hack, I thhink the issue should be opened upstream on vue-next, and fix it there.

If we ever end up merging this hack in VTU, maybe we should consider not removing the Symbol, but replacing its value with a string equivalent, so the stub will properly reflect that there was a Symbol.

cexbrayat · 2021-11-18T09:31:56Z

I opened a PR to fix this in vue-next instead vuejs/core#4964

If that gets merged we won't need this PR, and a simple upgrade to the latest vue-next will solve the issue.
Otherwise, we'll go with Lachlan's hack 😉

lmiller1990 · 2021-11-18T23:34:25Z

Nice, I added a comment to your PR vuejs/core#4964

I like your fix, although Vue core has so many open PRs - if it won't be merged for a while, we could use this until that PR is merged, then change to that implementation.

We should ping Evan for a review on that PR, as well as the jest.spyOn fix.

cexbrayat

As the PR in vue-next was not merged, let's go with the hacky patch in VTU 😉

cexbrayat · 2021-12-06T06:54:24Z

src/stubs.ts

+  const $props = props as unknown as ComponentObjectPropsOptions
+  return Object.keys($props).reduce((acc, key) => {
+    if (typeof $props[key] === 'symbol') {
+      return acc


WDYT of stringifying the Symbol instead of completely removing it? It would then show in the snapshot. (a simple $props[key].toString() should do the trick)

+1

given that this is only an issue when shallow mounting, I guess stringifying makes sense as people use shallow mounting to do things such as snapshotting or checking against a prop value. I never use it, but I guess stringifying the symbol aligns with the same mental model of "exploring" the rendered stuff

tests/props.spec.ts

cexbrayat · 2021-12-06T06:57:09Z

src/stubs.ts

@@ -62,7 +86,7 @@ export const createStub = ({
  })
 }

-const createTransitionStub = ({ name }: StubOptions) => {
+const createTransitionStub = ({ name, propsDeclaration }: StubOptions) => {


Are the changes to createTransitionStub necessary? I fail to see the link. If it is necessary maybe add a test case with a transition?

lmiller1990 · 2021-12-13T09:29:48Z

@cexbrayat @afontcu updated 👍

cexbrayat · 2021-12-13T09:40:12Z

src/stubs.ts

@@ -42,6 +43,17 @@ const shouldNotStub = (type: ConcreteComponent) => doNotStubComponents.has(type)
 export const addToDoNotStubComponents = (type: ConcreteComponent) =>
  doNotStubComponents.add(type)

+const removeSymbols = <T = ComponentPropsOptions>(props: T): T => {


nit: this could be renamed to stringifySymbols as it is not removing them anymore?

ooc: is the generic type T necessary here? As it is only called once, can't we use ComponentPropsOptions directly?

Removed that

cexbrayat · 2021-12-13T09:40:59Z

src/stubs.ts

+    // something like `el.setAttribute('val', Symbol())` which is not valid and
+    // causes an error.
+    // Only a problem when shallow mounting. For this reason we iterate of the
+    // props that will be passed and remove any that are symbols.


description is not accurate anymore: this should mention that they are stringified

cexbrayat · 2021-12-13T09:41:27Z

src/stubs.ts

+    // causes an error.
+    // Only a problem when shallow mounting. For this reason we iterate of the
+    // props that will be passed and remove any that are symbols.
+    const propsWithoutSymbols = removeSymbols(ctx.$props)


nit: same here, the variable could be renamed

cexbrayat · 2021-12-13T09:41:49Z

src/stubs.ts

@@ -164,7 +188,8 @@ export function stubComponents(
    if (type === Transition && 'transition' in stubs && stubs['transition']) {
      return [
        createTransitionStub({
-          name: 'transition-stub'
+          name: 'transition-stub',
+          propsDeclaration: {}


If the option is reverted to optional, this can be removed.

cexbrayat · 2021-12-13T09:41:56Z

src/stubs.ts

@@ -179,7 +204,8 @@ export function stubComponents(
    ) {
      return [
        createTransitionStub({
-          name: 'transition-group-stub'
+          name: 'transition-group-stub',
+          propsDeclaration: {}


cexbrayat · 2021-12-13T09:44:40Z

src/stubs.ts

@@ -21,7 +22,7 @@ import { Stub, Stubs } from './types'

 interface StubOptions {
  name: string
-  propsDeclaration?: ComponentPropsOptions
+  propsDeclaration: ComponentPropsOptions


is this necessary? Can't that be left optional, and have a default value in the stubs?

Done, fixed all comments, thanks for the review!

lmiller1990 added 4 commits November 17, 2021 22:20

update

d11f04b

repro

84670a6

fix

4ddcd22

fix bug with symbols

e0f372f

lmiller1990 changed the title ~~Issue 1076~~ fix: remove symbol props from stubs Nov 18, 2021

lmiller1990 mentioned this pull request Nov 18, 2021

bug: vue jest utils, shallowMount does not work when property values are symbols ionic-team/ionic-framework#24181

Closed

6 tasks

lint

f051b60

cexbrayat reviewed Nov 18, 2021

View reviewed changes

xanf force-pushed the master branch from 11f64fa to 125ea2f Compare November 30, 2021 07:26

cexbrayat requested changes Dec 6, 2021

View reviewed changes

render symbol in stub

63514b4

lmiller1990 requested review from afontcu and cexbrayat December 13, 2021 09:29

lint

deee71e

cexbrayat reviewed Dec 13, 2021

View reviewed changes

lmiller1990 added 2 commits December 13, 2021 22:25

revert un-needed changes

232bf57

prettier

a54b1da

afontcu approved these changes Dec 13, 2021

View reviewed changes

cexbrayat approved these changes Dec 13, 2021

View reviewed changes

cexbrayat merged commit 9ef6ad8 into master Dec 13, 2021

cexbrayat deleted the issue-1076 branch December 13, 2021 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove symbol props from stubs #1086

fix: remove symbol props from stubs #1086

lmiller1990 commented Nov 18, 2021

cexbrayat left a comment

cexbrayat commented Nov 18, 2021

lmiller1990 commented Nov 18, 2021

cexbrayat left a comment

cexbrayat Dec 6, 2021

afontcu Dec 9, 2021

lmiller1990 Dec 13, 2021

cexbrayat Dec 6, 2021

lmiller1990 Dec 13, 2021

lmiller1990 commented Dec 13, 2021

cexbrayat Dec 13, 2021

cexbrayat Dec 13, 2021

lmiller1990 Dec 13, 2021

cexbrayat Dec 13, 2021

cexbrayat Dec 13, 2021

cexbrayat Dec 13, 2021

cexbrayat Dec 13, 2021

cexbrayat Dec 13, 2021

lmiller1990 Dec 13, 2021

fix: remove symbol props from stubs #1086

fix: remove symbol props from stubs #1086

Conversation

lmiller1990 commented Nov 18, 2021

cexbrayat left a comment

Choose a reason for hiding this comment

cexbrayat commented Nov 18, 2021

lmiller1990 commented Nov 18, 2021

cexbrayat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 commented Dec 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment