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

Thanks for the effort to the repo, I made some super tiny enhancements for you to check #5

Merged
merged 5 commits into from Apr 23, 2022

Conversation

zhoujinfu
Copy link

  1. setup simple unit tests with vitest.
  2. change devDep node-html-parser as dep to fix module NOT FOUND errors.
  3. i met a import.env.LEGACY undefined errors in plugin.configResolved reproduce with 9f2ace0 and fixed with aa2e90f.

@zhoujinfu zhoujinfu changed the title Thanks for the effort to the repo, I made a special enhancements for you to check Thanks for the effort to the repo, I made some super tiny enhancements for you to check Apr 23, 2022
@chenxch
Copy link
Owner

chenxch commented Apr 23, 2022

This is all great, thank you so much for your contribution.

tests\__snapshots__ I think it should be ignored by default, what do you think?

@zhoujinfu
Copy link
Author

Maybe we should keep the snapshots in repo and reviewed as part of your code review process. As quote from jest doc:
https://jestjs.io/docs/snapshot-testing#snapshot-testing-with-jest

The snapshot artifact should be committed alongside code changes, and reviewed as part of your code review process. Jest uses pretty-format to make snapshots human-readable during code review. On subsequent test runs, Jest will compare the rendered output with the previous snapshot. If they match, the test will pass. If they don't match, either the test runner found a bug in your code (in the component in this case) that should be fixed, or the implementation has changed and the snapshot needs to be updated.

@zhoujinfu
Copy link
Author

I got another issue with the plugin in truly project. When we using vite-plugin-dynamic-base along with vite-plugin-pwa. It seems that we should both using the publicPath of vite-plugin-dynamic-base with base of vite-plugin-pwa. Any idea to go through it?

@chenxch
Copy link
Owner

chenxch commented Apr 23, 2022

Our snapshots will be a bit special. The content involves file directories. Everyone’s device is different, and the project storage address is different, so there will be different answers. At the time, I didn’t think it was a big problem.

@zhoujinfu
Copy link
Author

Our snapshots will be a bit special. The content involves file directories. Everyone’s device is different, and the project storage address is different, so there will be different answers. At the time, I didn’t think it was a big problem.

Well, i'll add shots to .gitignore for this PR

@chenxch
Copy link
Owner

chenxch commented Apr 23, 2022

We are actually using base and publicpath as a match.

@chenxch chenxch merged commit 72aa850 into chenxch:main Apr 23, 2022
Copy link
Author

@zhoujinfu zhoujinfu left a comment

Choose a reason for hiding this comment

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

Remove the snapshots/ and add __snapshots__/ as git igored

@chenxch
Copy link
Owner

chenxch commented Apr 23, 2022

@zhoujinfu Is it convenient to file an issue to discuss your problem?

@chenxch
Copy link
Owner

chenxch commented Apr 23, 2022

@zhoujinfu Thanks for your contribution, a new version has been released.

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

Successfully merging this pull request may close these issues.

None yet

2 participants