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

Make node-forge an optional dependency #8532

Closed
4 tasks done
patak-dev opened this issue Jun 10, 2022 · 6 comments
Closed
4 tasks done

Make node-forge an optional dependency #8532

patak-dev opened this issue Jun 10, 2022 · 6 comments
Labels
enhancement New feature or request p3-significant High priority enhancement (priority)
Milestone

Comments

@patak-dev
Copy link
Member

Clear and concise description of the problem

@sapphi-red made Terser an optional dep with:

@TrySound suggested that we do the same with node-forge. It is a big dependency (1.5MB) and it is only needed when creating an HTTP cert:

Suggested solution

We have two options:

  1. Make the dep optional leaving the functionality as-is. If the user doesn't include a cert, we'll ask them to install node-forge.
  2. Move the auto-generated HTTP cert logic to a new @vitejs/plugin-basic-ssl. See comment from @dominikg here fix: pregenerate https certificate #6173 (comment). Add documentation to explain how to properly generate certs and do a proper SSL setup (and link to the basic SSL plugin as an option, but not even the best)

I think 2. is better here. Opening the issue in case someone wants to work on it, I'll get to it if not before v3.

Alternative

No response

Additional context

No response

Validations

@patak-dev patak-dev added this to the 3.0 milestone Jun 10, 2022
@patak-dev patak-dev added enhancement New feature or request p3-significant High priority enhancement (priority) and removed enhancement: pending triage labels Jun 10, 2022
@winderica
Copy link

What about the third option - migrate from node-forge to @peculiar/x509? The latter

  1. has a much smaller publish size (379 KB),
    • for node 14, webcrypto is not natively supported, and an additional dependency @peculiar/webcrypto (192 KB) is required
  2. is written in TypeScript and comes with first-party type definitions,
  3. provides friendlier APIs.

If this makes sense, I am willing to submit a PR.

Of course, this option can be taken together with option 1 or option 2, but I am afraid I don't have much time to work on them :(

@patak-dev
Copy link
Member Author

Interesting. It looks like the library is quite actively maintained. @winderica this seems like a good option to reduce the size, we could discuss it in a PR to see how it works.
For v3, it is still important to do option 1 or 2 so we also avoid the extra 570kb (it is half of node-forge) but still big compared to the current size of Vite. So the priority should be to do that, but you can work in parallel to improve 1 or 2 if you would like to do that.

@sapphi-red
Copy link
Member

I took some data.

before #8584 (d1ba059)

npm notice === Tarball Details ===
npm notice name:          vite
npm notice version:       3.0.0-alpha.11
npm notice filename:      vite-3.0.0-alpha.11.tgz
npm notice package size:  888.4 kB
npm notice unpacked size: 3.7 MB
npm notice shasum:        fb4e7177d4f88b1bf964b91eb3bb6a10e8945245
npm notice integrity:     sha512-HsuLVUIG9I21P[...]Z7a+LbmnRU2Pg==
npm notice total files:   42

with #8584 (ac680a1)

npm notice === Tarball Details ===
npm notice name:          vite
npm notice version:       3.0.0-alpha.11
npm notice filename:      vite-3.0.0-alpha.11.tgz
npm notice package size:  835.8 kB
npm notice unpacked size: 3.7 MB
npm notice shasum:        54092e618b46cbff501ac9dc0b068abaacb49403
npm notice integrity:     sha512-57bVTQhMFlmLI[...]mWxKiL2j4biHw==
npm notice total files:   43

with #8584 (ac680a1) and removed @peculiar/webcrypto

npm notice === Tarball Details ===
npm notice name:          vite
npm notice version:       3.0.0-alpha.11
npm notice filename:      vite-3.0.0-alpha.11.tgz
npm notice package size:  814.2 kB
npm notice unpacked size: 3.6 MB
npm notice shasum:        839a3344541b26c99ffd909e7c38f3521961189b
npm notice integrity:     sha512-4VFC8lq7KiWx2[...]CxAI4D0f+BV2Q==
npm notice total files:   42

removed src/node/certificate.ts completely

npm notice === Tarball Details ===
npm notice name:          vite
npm notice version:       3.0.0-alpha.11
npm notice filename:      vite-3.0.0-alpha.11.tgz
npm notice package size:  743.2 kB
npm notice unpacked size: 3.1 MB
npm notice shasum:        1687d6312174b69bf0d0526b7b462d5698726a05
npm notice integrity:     sha512-YZK8iXRWgZDl4[...]DPuDUhydhCP/Q==
npm notice total files:   42

@patak-dev
Copy link
Member Author

Thanks for this data @sapphi-red. Maybe we should wait for swapping node-forge once we can drop Node 14? The package size diff is still not bad though (a ~6% drop).
I think I would feel a lot more comfortable with doing this change if we move certificate.ts to its own plugin, and swap node-forge there.

@winderica
Copy link

Maybe we should wait for swapping node-forge once we can drop Node 14?

That makes sense to me.

@tan9
Copy link

tan9 commented Jun 29, 2022

+1 for making node-forge optional or migrating to another implementation.

node-forge is dual-licensed under BSD and GPLv2, and because of the presence of GPL, the use of vite may be prohibited under some organizations like mine.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request p3-significant High priority enhancement (priority)
Projects
Archived in project
Development

No branches or pull requests

4 participants