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: cache certificate #3642

Merged
merged 3 commits into from Jul 7, 2021
Merged

feat: cache certificate #3642

merged 3 commits into from Jul 7, 2021

Conversation

wmzy
Copy link
Contributor

@wmzy wmzy commented Jun 2, 2021

Description

If we enable https with the config below:

export default defineConfig({
  server: {
     https: true,
  },
});

Browser will throw the NET::ERR_CERT_AUTHORITY_INVALID error ervery time vite restart.

This PR cache the certificate to file system to avoid the boring error.

Additional context

I do not know if I can use the config.cacheDir directory to cache this.
It seem that it only use for build data and maybe clean by --force option.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@patak-dev
Copy link
Member

Thanks for the PR. We'll discuss this feature with Evan to get approval next week.
I think it makes sense to use the cacheDir for this as you did. Do you see a problem with --force also deleting the cert cache?

@wmzy
Copy link
Contributor Author

wmzy commented Jun 2, 2021

@patak-js --force clean the cache cert file just as no this feature. But it is not a serious problem.

I think it is a good way that give some sub directories which do not clean by --force for vite plugins and other use case.
So that plugins no need add another cacheDir config for users.

@patak-dev
Copy link
Member

I think it is a good way that give some sub directories which do not clean by --force for vite plugins and other use case.
So that plugins no need add another cacheDir config for users.

Yes, I agree. But I don't know if there is a need to add the fine-grained options at this point. But I think we may end up with --clean-optimized-deps, --clean-https-cert, etc at one point.

@Shinigami92 Shinigami92 added the p2-nice-to-have Not breaking anything but nice to have (priority) label Jun 12, 2021
antfu
antfu previously approved these changes Jun 24, 2021
@patak-dev
Copy link
Member

patak-dev commented Jun 26, 2021

We discussed this with Evan and we have his approval to merge this feature. But I just saw that there were some previous discussions about this in the past between Evan and @underfin in #276. Looks like the PR was closed after the v2 rewrite, but the feature was also considered good to have at that point. But there were some talks about storing IP addresses in the cert. @wmzy would you review that discussion? @underfin could you review this new PR?

@patak-dev patak-dev requested a review from underfin June 26, 2021 11:57
@wmzy
Copy link
Contributor Author

wmzy commented Jun 28, 2021

@patak-js This pr does not change the cert creation logic, but only add the cache to prevent the NET::ERR_CERT_AUTHORITY_INVALID error every time vite is restarted.

If that storing IP addresses in the cert is needed, it is better another pr.

refact: empty catch style.

Co-authored-by: Shinigami <chrissi92@hotmail.de>
@wmzy
Copy link
Contributor Author

wmzy commented Jul 7, 2021

Hi @patak-js @antfu ,can you help to merge this pr?
Do I need to make any changes?

@patak-dev patak-dev merged commit 5dd670f into vitejs:main Jul 7, 2021
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
Co-authored-by: Shinigami <chrissi92@hotmail.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants