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: support <script setup> in Vue 2.7 (supersedes #483) #489

Merged
merged 12 commits into from
Sep 15, 2022

Conversation

kiroushi
Copy link
Contributor

This PR basically borrows @FelixGraf work in #483 and merges snapshot fixes from #486 into it.

The source map in script setup generated code didn’t have a trailing \n, so the __options__ var declaration was being inlined with it.

I removed the source map from the generated Vue 2 code, I don’t know if this is a necessary change, and someone with more insight on the library might shed some light.

@ndom91
Copy link

ndom91 commented Aug 18, 2022

Great stuff! Works for me. This was breaking our <setup script> + Vue 2 related builds as well.

@lmiller1990
Copy link
Member

Nice, if CI is green I can merge and release 28.1.1.

@lmiller1990
Copy link
Member

Looks like we've got some failures - can you take a look @kiroushi?

@FelixGraf
Copy link
Contributor

Thanks for taking over my changes, I haven't had the time to continue them. One thing that was mentioned in my PR is this comment:

@vue/compiler-sfc isn't listed as a dependency of this package, so this import is unsound. There is a similar existing issue for the vue-template-compiler import as well (which should probably have been listed as an optional peer dependency).

People using Vue < 2.7 won't have the @vue/compiler-sfc dependency installed by default, as it's only available for Vue >= 2.7. This might be a breaking change, as users with older Vue versions should still be able to use vue-jest in a new patch/minor version. 🤔

@kiroushi
Copy link
Contributor Author

People using Vue < 2.7 won't have the @vue/compiler-sfc dependency installed by default, as it's only available for Vue >= 2.7. This might be a breaking change, as users with older Vue versions should still be able to use vue-jest in a new patch/minor version. 🤔

You’re absolutely right. I will spend more time this weekend so we can make this back/forth compatible 👍

@kiroushi
Copy link
Contributor Author

@lmiller1990 can you approve the workflow and see if it’s passing now? I will work on trying to make this backwards compatible too.

@dten
Copy link

dten commented Aug 20, 2022

for me 28.1.0 fixed the source maps and this MR breaks them again

@lmiller1990
Copy link
Member

lmiller1990 commented Aug 23, 2022

CI is passing. Hmm - I wonder if @dten can share his setup - we ideally don't want to merge and release a fix that breaks the previous release of 28.1.0. 🤔 WDYT @kiroushi?

@dten
Copy link

dten commented Aug 23, 2022

In away from PC for a week now 🙈 if it's got tests and working for everyone else then go for it. There's a chance I messed up testing the pr locally. I only have a work repo so not able to share without some effort cutting down.

@kiroushi
Copy link
Contributor Author

@lmiller1990 I didn’t have time to look into the Vue 2.6 vs Vue 2.7 issue. As @FelixGraf stated, these changes will break compatibility with any Vue version not including 2.7 @vue/compiler-sfc.

@kiroushi
Copy link
Contributor Author

@dten @lmiller1990 I think this should be good to go now.

I’ve cloned https://github.com/morellexf26/tuto, changed Vue version to 2.6.14, and modified the test to take a snapshot.

Then I:

  • Changed @vue/vue2-jest to 28.1.0 and added "jest-environment-jsdom": "28.0.2". Ran test and was passing.
  • Changed @vue/vue2-jest to this branch. Ran test and was passing (snapshots were OK).
  • Changed vue dependency to 2.7.10 and changed SpotifyTracker.vue component to setup syntax. Ran tests and they were passing, snapshots were still OK.

I think I’ve covered all the cases, let me know if you find an edge case.

cc @FelixGraf too

@dten
Copy link

dten commented Aug 24, 2022

That's with --no-cache to jest? For me I was getting misleading results because it was caching the old maps despite package version changing

Comment on lines 17 to 23
try {
compilerUtils = require('@vue/compiler-sfc')
isVue27 = true
} catch (e) {
compilerUtils = require('@vue/component-compiler-utils')
}
Copy link
Contributor

@thebanjomatic thebanjomatic Aug 26, 2022

Choose a reason for hiding this comment

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

Should @vue/compiler-sfc be listed as an optional peer dependency?

Alternatively, if we just require('vue/compiler-sfc') then we would be able to leverage the existing vue 2.7 import since it gets repackaged from the vue dependency instead of needing to explicitly reference this other package.

https://github.com/vuejs/vue/blob/main/package.json#L28-L31

@@ -8,8 +8,18 @@ const stripInlineSourceMap = require('./utils').stripInlineSourceMap
const getCustomTransformer = require('./utils').getCustomTransformer
const loadSrc = require('./utils').loadSrc
const babelTransformer = require('babel-jest').default
const compilerUtils = require('@vue/component-compiler-utils')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thebanjomatic this import used to be here anyway, without being declared explicitly in the peer dependencies. My exposure to this codebase is limited so I am not really aware of the consequences of these changes given the amount of permutations we can have in regards of vue-jest,jest, and vue versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

The thing is that its already a peer dependency, the project just isn't declaring it in the package.json manifest. This would have issues if you were using yarn pnp for example because it is trying to import a package that isn't declared and you would need to use packageExtensions to inject those declarations as consumers of the library.

If you don't want to make the change in this PR, that's fine. I might pursue a separate PR later to clean up these undeclared peerDependencies and to fully optionalize vue-template-compiler so that you don't need to install that separately for vue 2.7 also.

I do think vue/compiler-sfc is still the better import than @vue/compiler-sfc though since we already have the vue peer dependency declared and this appears to be the preferred way to import compiler-sfc now that it ships with the vue dependency out of the box.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thebanjomatic that’s a fair point. I can change this PR. Will update it later.

Copy link
Member

Choose a reason for hiding this comment

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

LMK when it's ready for a re-review.

Providing compat for all versions is really hard and tedious... :|

Copy link
Member

Choose a reason for hiding this comment

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

PS just released v29 for Jest 29

You might need to rebase. If this merges, the changes will be in v29 - we can back port to v28, but it's a lot of work to maintain multiple versions, I generally ask people to update to the latest Jest.

https://github.com/vuejs/vue-jest/releases/tag/v29.0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lmiller1990 sorry I’ve been kinda busy last week, so what’s your recommendation? Make this change only compatible with Jest 29?

@kaizige10
Copy link

I have tried this PR in my local project. Most of the time it's OK. Except when I use both <script> and <script setup> in one component to define component name, I find the <script setup> do not execute.

My component is:

<script>
console.log('in <script>');
export default {
    name: 'LiveHeader',
};
</script>

<script setup>
console.log('in <script setup>');
</script>

jest log is:

  console.log
    in <script>

      at Object.log (vpages/monitor/components/LiveHeader.vue:2:9)

 FAIL  test/monitor/liveheader.test.js
  LiveHeader
    × check title (5 ms)
    × check popular and onlineUser (4 ms)

You will see only in <script> has been logged. So I think the <script setup> will not take effect when I use <script>. And I don't know why. Pls check. @kiroushi @FelixGraf

@kiroushi kiroushi requested review from lmiller1990 and thebanjomatic and removed request for lmiller1990 September 7, 2022 22:38
Copy link
Contributor

@thebanjomatic thebanjomatic left a comment

Choose a reason for hiding this comment

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

I tested this in a project of mine and it appears to be working correctly, and the concerns I raised earlier have all been addressed. From my perspective this looks good to me, but I did have one question still regarding the sourcemaps, but I wasn't sure how to test that.

"mappings": ";;;;;;AAAA;AACA;AACA;AACA;AACA;AACA;AACA;AACA;AACA;AACA;AACA;AACA;AACA;AACA;AACA;AACA;AACA;AACA;AACA;AACA;AACA;AACA;eAEe;AACbA,MAAI,EAAE,OADO;AAEbC,UAAQ,EAAE;AACRC,kBAAc,EAAE,SAASA,cAAT,GAA0B;AACxC,aAAO;AACLC,WAAG,EAAE,KAAKC,OADL;AAELC,YAAI,EAAE,CAAC,KAAKD,OAFP;AAGLE,cAAM,EAAE,KAAKF;AAHR,OAAP;AAKD;AAPO,GAFG;AAWbG,MAAI,EAAE,SAASA,IAAT,GAAgB;AACpB,WAAO;AACLC,SAAG,EAAE,4BADA;AAELJ,aAAO,EAAE;AAFJ,KAAP;AAID,GAhBY;AAiBbK,SAAO,EAAE;AACPC,eAAW,EAAE,SAASA,WAAT,GAAuB;AAClC,WAAKN,OAAL,GAAe,CAAC,KAAKA,OAArB;AACD;AAHM;AAjBI,C",
"names": [
Object {
"file": "/home/runner/work/vue-jest/vue-jest/e2e/2.x/basic/components/Basic.vue",
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems weird for this to have switched to an absolute path. I would expect this to still be a relative path, but its possible this doesn't really matter much since the source-maps are then consumed by jest on the same machine. Possibly there could be issues if you are sharing a jest cache? Ultimately I don't know enough about this to make a judgement call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @thebanjomatic. I don’t think the file property should be problematic, but I will revert it back to the original. I don’t know at which point it did change (because that’s not even the path of my local env, so I am guessing I took over these changes from Felix too).

@aldarund
Copy link

aldarund commented Sep 14, 2022

It breaks script setup with @nuxtjs/composition-api in a new different way than general problem before this pr. With this version it ends up with.

SyntaxError: Cannot use import statement outside a module
 Details:

    C:\Dev\PycharmProjects\StampinUp.Ordering.Frontend\node_modules\@nuxtjs\composition-api\dist\runtime\index.mjs:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){import { getCurrentInstance as

That happens if just do something like

<script setup>
import { useAsync } from '@nuxtjs/composition-api'
useAsync(() => {
  const k = 3
})
</script>

Which works without this pr.

UPD. need adding to jest config

moduleNameMapper: {
  '^@nuxtjs/composition-api$': path.join(
      __dirname,
      'node_modules/@nuxtjs/composition-api/dist/runtime/index.js'
    )
    }

Which fixes this issue and seems to work fine

@lmiller1990
Copy link
Member

Sorry for going MIA, some other things came up that are more pressing than OSS work for me, but I want to get this out, let's do it. Looks like this has been tested decently well by this point.

Last thing we might want to do is document @aldarund's findings somewhere for Nuxt users?

@@ -10639,7 +10677,7 @@ vue-template-es2015-compiler@^1.9.0:
"vue2-sass-importer-lib@file:e2e/2.x/sass-importer/lib":
version "1.0.0"
dependencies:
vue2-sass-importer-sass-lib "file:../../.cache/yarn/v6/npm-vue2-sass-importer-lib-1.0.0-1087f856-8699-4ac6-a0fa-2288988256c2-1656107462461/node_modules/sass-lib-v1"
vue2-sass-importer-sass-lib "file:../../Library/Caches/Yarn/v6/npm-vue2-sass-importer-lib-1.0.0-7ea9c798-4187-4843-8438-890703751297-1658071612744/node_modules/sass-lib-v1"
Copy link
Member

Choose a reason for hiding this comment

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

Weird. looks MacOS specific

@lmiller1990
Copy link
Member

lmiller1990 commented Sep 15, 2022

Actually let's ship it, docs can come later - best unblock the PR and get this out in the wild for some real usage to see if we've broken anything. Most people should pin their vue-jest version so I don't think this should be disruptive.

Looks like CI is failing. I'll take look.

@lmiller1990
Copy link
Member

hmm I guess something somewhere changed the snapshot serializing for arrays slightly: 4e93580

Looks good now, let's ship it.

@lmiller1990 lmiller1990 merged commit 7f2287c into vuejs:master Sep 15, 2022
@lmiller1990
Copy link
Member

Shipped https://github.com/vuejs/vue-jest/releases/tag/v29.1.0, nice job everyone.

@dennybiasiolli
Copy link

Actually let's ship it, docs can come later - best unblock the PR and get this out in the wild for some real usage to see if we've broken anything. Most people should pin their vue-jest version so I don't think this should be disruptive.

Looks like CI is failing. I'll take look.

Thanks everyone for the update, now it's working (almost) fine.
I just noticed a little error during jest tests when trying to spy on a component method like in this example:

const wrapper = shallowMount(CounterLocal)
const spyIncrement = jest.spyOn(wrapper.vm, 'increment')
wrapper.find('button').trigger('click')
expect(spyIncrement).toHaveBeenCalled()

Tests are breaking with Cannot spy the increment property because it is not a function; undefined given instead

I found a workaround with

- const spyIncrement = jest.spyOn(wrapper.vm, 'increment')
+ const spyIncrement = jest.spyOn(wrapper.vm._setupState, 'increment')

Example repository here: https://github.com/dennybiasiolli/vue-migration-2-to-3, workaround applied to component
tests/unit/views/CounterLocal.spec.js

@thebanjomatic
Copy link
Contributor

@dennybiasiolli that seems like it is working as expected with script setup. Components are closed by default. If you need to unit test increment, you should expose it using the defineExpose({increment}) macro.

@dennybiasiolli
Copy link

@thebanjomatic thanks, you are right about that. I tried with defineExpose({increment}) but no luck in spying on the method. It seems to call something else.
I think I need to find another way to test for buttons click and methods call 😄

@thebanjomatic
Copy link
Contributor

thebanjomatic commented Sep 15, 2022

Ah yes.... I can reproduce, and I think your workaround makes sense (though I have the same problem with it that I have with defineExpose). The compiled code from <script setup> is likely just making direct function calls to functions within a closure. When you use defineExpose, it provides an alias to those functions on the vm. Spying on the vm is only going to rewrite that alias and all the internal references to that function within script setup will still go directly to that implementation instead of your spy.

A simpleish workaround that I'm not in love with is to have an object that has all the methods you need to spy on and expose that instead.

<template>
  <div>
    <p>Count is: {{ count }}</p>
    <button @click="() => hooks.increment()">Increment</button>
  </div>
</template>
import {ref} from 'vue';

const count = ref(0);

const hooks = {
  increment() { count.value++ }
}

defineExpose({hooks});

Then if you do this it should hopefully hit your spy:

const spy = jest.spyOn(wrapper.vm.hooks, 'increment').mockReturnValue(undefined);

@lmiller1990
Copy link
Member

lmiller1990 commented Sep 15, 2022

I would really recommending finding an alternative testing strategy here. After years of triaging Test Utils and Jest, I learned

  • testing implementation details is brittle
  • internals can (and do) change in Vue (even minors)
  • coupling to testing framework (Jest) is also not ideal

In your example you could do something like

const wrapper = mount(Component)
await wrapper.find('button').trigger('click')
expect(wrapper.find('p').text()).toEqual('Count is 1')

I'm normally using Cypress now which does something similar:

cy.mount(Component)
cy.get('button').click()
cy.get('p').should('contain.text', 'Count is 1')

It's good since it forces you to avoid internals - they are not even available.

I wouldn't use p (brittle, changing to div breaks your test, despite no functional different to user). Consider something better like a data-test or role selector.

@dennybiasiolli
Copy link

Thank you @lmiller1990 , your example is correct and it follows the vuejs guidelines about testing:

When testing, remember to test what a component does, not how it does it.

I need to rethink a few test cases, but it was totally a bad habit of mine.

BTW the repo I linked is an example I will use in a talk at VueDay Verona explaining how to upgrade from Vue 2.6 to Vue 3.x passing through 2.7. Your recent update (29.1) is helping a lot in this process, thank you very much for this update!

@lmiller1990
Copy link
Member

No problem, but don't thank me, most of the heavy lifting for this was done by the others in this PR :)

@@ -29,7 +28,7 @@ module.exports = function(allBlocks, filename, config) {
const codeStr = applyTransformer(
transformer,
blocks,
vueOptionsNamespace,
componentNamespace,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a breaking change (bug?) introduced for custom block processors. This import in process.js is just undefined, because ./constants does not have vueComponentNamespace:

const vueComponentNamespace = require('./constants').vueComponentNamespace

This means that the vueOptionsNamespace in the custom processor will always be undefined. This currently breaks vue-i18n-jest for me here:

https://github.com/intlify/vue-i18n-jest/blob/afaa5c3d4dd2e54760a5ef73cd2829672348cf25/src/process.ts#L43

Copy link
Member

Choose a reason for hiding this comment

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

Argh, sorry. Can you file an issue with a minimal repro?

Bonus points if you want to make a PR!!

Copy link
Member

Choose a reason for hiding this comment

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

Edit: you already made a PR!

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.