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

Bundle dts files into single file #470

Closed
wants to merge 4 commits into from
Closed

Bundle dts files into single file #470

wants to merge 4 commits into from

Conversation

mrmlnc
Copy link
Owner

@mrmlnc mrmlnc commented Dec 13, 2024

  • build: bundle dts files
  • build: explicitly include build files into package
  • build: move production dependencies to dev dependencies

What is the purpose of this pull request?

What changes did you make? (Give an overview)

@benmccann
Copy link

Is the intention to bundle only dts files per the PR title? It looks like this is also bundling the JS, but I'm not sure if that's just temporary. If the JS gets bundled how would security vulnerabilities in dependencies be handled? (e.g. https://security.snyk.io/vuln/SNYK-JS-BRACES-6838727). And of course dependencies like micro match/picomatch would no longer dedupe

@mrmlnc
Copy link
Owner Author

mrmlnc commented Jan 2, 2025

@benmccann, to be more precise, the bundling of JS files is already in the master branch. In this pull request, I clarified the patterns and enabled bundling of dts files to avoid distribute dozens of dts files.

And of course dependencies like micro match/picomatch would no longer dedupe

Yes, I understand that bundling is unconventional for node.js world solution. However, this reduces the required-time of the package by half (~24ms -> ~10ms). I don't think it's a lot, but I would like to try it out in the master branch before the release. I will probably roll out these changes completely or I will bundle some of the dependencies.

According to my estimates, this is not as much as you might think. Given the speed of the release of new versions, I think it will be 1-3 copies.

If the JS gets bundled how would security vulnerabilities in dependencies be handled?

I can't think of any real CVE that might affect this package. All the CVEs about the braces and micromatch package assumed that the fast-glob package should be available to the user directly in your application. Otherwise, the attacker must have access to your file system. And in this case, the CVE in fast-glob no longer matters.

Even in the case of CVE, the current rate of release of new dependency versions is ~1 time per year. This will require the release of a new version of this package, but I don't see any difficulties.

@ljharb
Copy link

ljharb commented Jan 2, 2025

If the deps aren’t listed, it will prevent proper SBOMs from being created, which many application consumers are legally required to have - whether you bundle the deps or not, i highly suggest leaving them listed as dependencies in package.json - that will mean the only downside is the need to rerelease + the dep graph dupes.

@mrmlnc
Copy link
Owner Author

mrmlnc commented Jan 3, 2025

I thought about it and decided that bundling dependencies in the library is not the best idea. To draw an analogy, the executable file is built for the application, not for the library. The introduction of dependency bundling complicates maintenance and introduces the risk of breakdowns at the bundling stage.

Yes, fast-glob does not have the best require-time since fast-glob itself and the @nodelib dependencies are written as modules instead of one large module. I've identified a couple of areas where I can optimize require-time. However, this will still be less effective than what bundling can provide. For CLI and other solutions where it is important to frequently run fast-glob on cold, I recommend using bundling when building them.

I plan to remove bundling from the master branch.

@mrmlnc mrmlnc closed this Jan 3, 2025
@mrmlnc mrmlnc mentioned this pull request Jan 3, 2025
@mrmlnc mrmlnc deleted the bundle_dts branch January 4, 2025 21:54
@mrmlnc mrmlnc restored the bundle_dts branch January 4, 2025 21:54
@mrmlnc mrmlnc deleted the bundle_dts branch January 4, 2025 21:55
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.

3 participants