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

Support custom domains #22

Merged
merged 3 commits into from
Jan 15, 2024
Merged

Conversation

gz65555
Copy link
Contributor

@gz65555 gz65555 commented Dec 29, 2023

This PR introduces the following features:

  1. Custom domains
  2. Custom cert directory
  3. Custom name of certification

Can be easily set by:

// vite.config.js
import basicSsl from '@vitejs/plugin-basic-ssl'

export default {
  plugins: [
    basicSsl({name: "test", domains: ["*.custom.com"], certDir: "/Users/.../.devServer/cert"})
  ]
}

Some essential options can make our local development more comfortable. In Chrome, there will not be unfriendly security warnings.

@sarim
Copy link

sarim commented Dec 31, 2023

It'd be nice to have this feature.
Building on top of it, providing rootCA to use for signing would be nice. (I have my own rootCA that is trusted in my devices) I can work on that after this is merged.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

The general idea sounds fine to me besides some comments below. We'd also need documentation for the new options though.

package.json Outdated Show resolved Hide resolved
src/certificate.ts Outdated Show resolved Hide resolved
src/certificate.ts Outdated Show resolved Hide resolved
@gz65555
Copy link
Contributor Author

gz65555 commented Jan 10, 2024

@bluwy Thank you for reviewing, modifications are done.

@gz65555
Copy link
Contributor Author

gz65555 commented Jan 11, 2024

The options are quite simple, so I just update readme and add some comments on the options.

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Maybe we should remove getCertificate from the public API? I don't know why we are exposing that, and we could directly pass the options as an object to it then.

@sarim
Copy link

sarim commented Jan 11, 2024

I guess getCertificate function is exported for testing?

@patak-dev
Copy link
Member

I guess getCertificate function is exported for testing?

It isn't used in the tests, no?

@sarim
Copy link

sarim commented Jan 11, 2024

@patak-dev

import { createCertificate } from '../src/certificate'

Update: Sorry, my bad. I mixed up the get and create.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Looks great to me! Thanks for making the changes. My only question I guess is maybe we should rename certDir to cacheDir. Since certDir could mean the directory if you generated the certificates yourself and we respect that (which we don't). But not too concerned of the naming either since it's a low maintenance package.

About the getCertificate, perhaps we can remove that in the next major if we want to.

@patak-dev
Copy link
Member

patak-dev commented Jan 15, 2024

Let's release a minor then to have this feature and respect user defined settings too from the other PR.

@bluwy, would you expand on

My only question I guess is maybe we should rename certDir to cacheDir. Since certDir could mean the directory if you generated the certificates yourself and we respect that (which we don't).

I didn't get that, certDir sounds good here. We should rename cacheDir to certDir in getCertificate to me 🤔

@patak-dev patak-dev merged commit 0a8665f into vitejs:main Jan 15, 2024
1 check passed
@bluwy
Copy link
Member

bluwy commented Jan 15, 2024

I didn't get that, certDir sounds good here. We should rename cacheDir to certDir in getCertificate to me 🤔

certDir technically is used more as a cache. And the other options didn't have the cert prefix, meaning they're implicitly cert related already. So I figured cacheDir was clearer.

@patak-dev
Copy link
Member

Ah, I see. cacheDir may confuse some folks with the Vite cache dir. But I understand your point. Let's leave it for now then 👍🏼

@gz65555 gz65555 deleted the feat/support-custom-domain branch January 15, 2024 10:20
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.

4 participants