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: add regex for vue SFC to prevent instrumenting style and templates #180

Merged
merged 8 commits into from
Feb 17, 2024

Conversation

hugo-daclon
Copy link
Contributor

Single file Components no longer have their html or css instrumented instead of their script

Closes #96 #178 #89

I was inspired by @sawmuraÏ's fix but didn't use it because I didn't know if it would break other stuff. Instead I added a regex to check if the id links to a vue SFC and if so check if it does not link to the script part.

I tested the RegExp on my computer with the following little script and it worked well:

const ids = [
  "C:/program/src/App.vue?vue&type=script&setup=true&xxxxx",
  "C:/program/src/App.vue?vue&type=script&setup=false&xxxxx",
  "C:/program/src/App.vue?vue&type=style&setup=true&xxxxx",
  "C:/program/src/App.vue?vue&type=template&setup=true&xxxxx",
  "C:/program/src/App.vue?vue&type=css&setup=true&xxxxx",
  "C:/program/src/App.vue?vue&type=html&setup=true&xxxxx",
  "C:/program/src/App.js",
  "C:/program/src/App.vue",
];
(() => {
  (() => {
    for (const id of ids) {
      const isVueSFC = /[?&]vue\b(?=&|$)/.test(id);
      const isVueSFCScript = /[?&]type=script\b(?=&|$)/.test(id);

      if (isVueSFC && !isVueSFCScript) {
        continue;
      }
      console.log(id);
    }
  })();
})();

PS: first ever PR on github so I'm not sure if I did things right or not.

@iFaxity
Copy link
Owner

iFaxity commented Feb 14, 2024

Hello @Nol-go, thanks for your PR.

Don't worry you did this correctly as i see it 😊.
I will test this to see if it works as intended.

You could try to test the fix through modifying your package json to point to the branch of your fork of the git repo. See this: https://docs.npmjs.com/cli/v10/configuring-npm/package-json#dependencies.

You can also use a local path if you have both the projects on your local machine. See this: https://docs.npmjs.com/cli/v10/configuring-npm/package-json#dependencies.

You can also link through npm, although i have had some issues using this on Windows. See this: https://docs.npmjs.com/cli/v10/commands/npm-link.

@hugo-daclon
Copy link
Contributor Author

Hello @Nol-go, thanks for your PR.

Don't worry you did this correctly as i see it 😊. I will test this to see if it works as intended.

You could try to test the fix through modifying your package json to point to the branch of your fork of the git repo. See this: https://docs.npmjs.com/cli/v10/configuring-npm/package-json#dependencies.

You can also use a local path if you have both the projects on your local machine. See this: https://docs.npmjs.com/cli/v10/configuring-npm/package-json#dependencies.

You can also link through npm, although i have had some issues using this on Windows. See this: https://docs.npmjs.com/cli/v10/commands/npm-link.

I managed to install it, I had with building the package because the package.json does not have a prepare script.
But,... IT DOES NOT WORK 😭 😭 😭
After logging localy, I found out that the id we get in the transform function is already stripped of the querry params I was checking for when building.
I looking into it more this afternoon (UTC+2 where I'm at)

@hugo-daclon hugo-daclon changed the title fix: add regex for vue SFC to prevent instrumenting style and templates draft: fix: add regex for vue SFC to prevent instrumenting style and templates Feb 14, 2024
@hugo-daclon
Copy link
Contributor Author

After further investigation, the id had the got rid of the query only for one of the three chunk so I changed the regex isVueSFC to check for .vue instead of ?vue. the downside is that anyOne with vueSFC that are not of the extension .vue won't have working coverage on their build file (shouldn't change anything on live-server instrumentation).

We could add a config value for this but I think it's to troublesome for a small minority of cases, I don't even know if this feature (replacing the vue SFC extension name by a custom one) is used by someone.

Also, I think the problem will still arise when there is multiple root <script> tags, though, if I remember well, it causes other problems with vueJS so it's not really an issue not to support this case.

@hugo-daclon
Copy link
Contributor Author

here is a local test result using the updated code with the following config.

istanbulPlugin({
    include: 'src/*',
    exclude: ['node_modules', 'test/'],
    cypress: true,
    requireEnv: true,
    forceBuildInstrument: true,
})

and running npx CYPRESS_COVERAGE=true vue-tsc --noEmit && vite build followed by npx vite preview

image

As you can see, the __coverage__ variable is correctly set for the App.vue file.

@hugo-daclon hugo-daclon changed the title draft: fix: add regex for vue SFC to prevent instrumenting style and templates fix: add regex for vue SFC to prevent instrumenting style and templates Feb 14, 2024
@hugo-daclon
Copy link
Contributor Author

@iFaxity PR is up for review

@hugo-daclon
Copy link
Contributor Author

As of this comment, the code does not seem to work for option API. I'll look into it later.

@hugo-daclon
Copy link
Contributor Author

@lukasbrzobohaty I added you to my project, and created a branch for you. You should be able to push code to it. But please do not touch the next branch as I need it to stay working for my team's project. I'll take care of merging your code to my fix once we are sure everything will work.

@hugo-daclon hugo-daclon marked this pull request as draft February 15, 2024 10:23
@hugo-daclon
Copy link
Contributor Author

I fixed @lukasbrzobohaty's issue with hybrid option and composition api.here are screenshots of the coverage in browser, and nyc's report after cypress ran on #178 linked project.

Screenshot_20240217_133443
image

@hugo-daclon hugo-daclon marked this pull request as ready for review February 17, 2024 12:55
@hugo-daclon
Copy link
Contributor Author

@iFaxity It all seems good to me, waiting for review and approval.

@iFaxity
Copy link
Owner

iFaxity commented Feb 17, 2024

Hi @Nol-go, nice job!

I will make some minor changes as some changes i do not want pushed to the repo.
Also i will make some changes to the formatting/style of the code.

@iFaxity
Copy link
Owner

iFaxity commented Feb 17, 2024

I will move the dependency changes as well as those are handled by dependabot.

@hugo-daclon
Copy link
Contributor Author

Hi @Nol-go, nice job!

I will make some minor changes as some changes i do not want pushed to the repo. Also i will make some changes to the formatting/style of the code.

Works fine for me, please do tell me when the patch will be versioned to npmjs.com.

@iFaxity
Copy link
Owner

iFaxity commented Feb 17, 2024

@Nol-go, when the PR gets merged and pushed to NPM the version will be added here as a comment, so just have notifications on and you will get a notification when the version is out :).

@iFaxity iFaxity merged commit c8a7ca7 into iFaxity:next Feb 17, 2024
3 checks passed
Copy link
Contributor

🎉 This PR is included in version 5.0.0-rc.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@iFaxity
Copy link
Owner

iFaxity commented Feb 17, 2024

@Nol-go an rc release is published on NPM. You should within minutes be able to use the 5.0.0-rc.4 or the next tag until it is released as a stable version.

For example using npm i --save-dev vite-plugin-istanbul@next or npm i --save-dev [email protected] you will get the lastest rc release with this patch.

Copy link
Contributor

github-actions bot commented Mar 9, 2024

🎉 This PR is included in version 6.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

"resolveFilename" causing same named paths in Vue files
2 participants