-
Notifications
You must be signed in to change notification settings - Fork 357
vendor bundled non-datadog dependencies #6958
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
base: master
Are you sure you want to change the base?
Conversation
Overall package sizeSelf size: 3.55 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 1.15.0 | 127.66 kB | 856.24 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6958 +/- ##
==========================================
- Coverage 84.83% 84.83% -0.01%
==========================================
Files 517 517
Lines 22050 22047 -3
==========================================
- Hits 18707 18704 -3
Misses 3343 3343 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
25ff3b5 to
2293587
Compare
|
Uuuh, i am ok with vendoring, but should we really commit obfuscated code like that ? I don't feel comfortable with that for security reasons, anyone can just hide |
Altering what you ship before shipping it is in itself a security risk, but it also means you're shipping something untested, so that's not an option.
This is already true for any update to one of our libraries, but I get what you're saying in that it would now be in the actual codebase. The problem is that some of our dependencies are already minified, so I'm not sure how we would be able to handle those as even if we disable minification on our end those would still not be human readable. Any suggestions for those cases? |
Sorry I meant bundling before testing of course 👍 like i don't want it to be bundling just as the last step before release. I mean bundling as a npm script to run after a dev install: when creating a new dev setup, when installing the package in the CI, before testing, etc, etc.
We have minified dependencies ??? what if we just stop using them ? sounds not great to have minified dependencies imo. |
I think that could work, I'd need to add some sort of bundling workflow that runs before all other workflows and stores the output in a cache or an artifact. I'm a bit worried about artifacts though given all the issues we've been having in system tests. @cbeauchesne Did we ever get an answer from GitHub as to what is going on? Generally speaking I'm not sure which option I prefer. We could always just have a validation step that ensures the bundle was not tempered with and push it to GitHub. This would avoid having to rebuild it everywhere every time, but at the cost of committed code that is not readable. This also has the benefit of the library being usable directly from Git as it is today. Or we can do as you say and build every time which ensures the build is always fresh and doesn't pollute the repo, but then everyone pays the cost of bundling every time and the library can no longer be installed directly from Git. I think because of the latter point I tend to lean towards committing the files to preserve that ability and simply add a validation step, but otherwise it's very similar as the build time is in milliseconds and not seconds or minutes. WDYT?
We don't decide how dependencies are packaged, and if we don't trust our dependencies our only option is to stop using all of them. Short of that, we just have to accept that there is no standard way to package and everyone does their own thing which may also change over time. |
they told us "works on my machine" |
f14532a to
4503806
Compare
Unfortunately no. At some point we'll need more data to prove them the impact this issue have. But TBH, it happened a little bit less those past weeks. |
Just until we can figure out why they are changing
What does this PR do?
Vendor bundled versions of non-Datadog dependencies.
Motivation
As we start supporting more and more non-server environments, our package size becomes increasingly problematic. Other than native addons (which are not included in non-server environments anyway), most of it comes from our dependencies, and most of their size comes from files that they package but never actually use. But even the code they do use is often unoptimized as it's not minified and is spread across many files. All of this not only makes their package bigger but also slower to load. By including vendored versions of our dependencies, we can make sure all of them are optimized in the way we want. This also has several other benefits:
@datadogare not included) then the tracer becomes self-contained. Once packed, it can be unpacked and already fully functional without any additional install. This also has the benefit of avoiding side-effects when mergingnode_modules, for example with the Lambda extension.Additional Notes
A next step would be to make the native addons a separate package so that they don't need to be removed when repackaging in a non-server environment and would instead simply not be installed in the first place.