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

Do not bundle dependencies #80

Closed
wants to merge 1 commit into from

Conversation

Andarist
Copy link
Contributor

gzip -c dist/rollup-plugin-typescript2.es.js | wc -c

goes down from 12107 to 7877, but what's more important is that debugging problems is easier, because it's immediately obvious which code is which

import replace from "rollup-plugin-re";

const pkg = require("./package.json");

const makeExternalPredicate = externalArr => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this handles things like 'colors/safe' automatically

@ezolenko
Copy link
Owner

ezolenko commented May 31, 2018

See discussion here for context and rationale of bundling: #70

If that doesn't convince you, I'll reconsider sometime before 3rd PR for the same thing :)

@Andarist
Copy link
Contributor Author

I hear your arguments, I'm just not convinced. My biggest concern (beside the duplication of code) is that the code's author and its origin is unobvious when debugging. Leftpad-like issues don't happen that often IMHO.

Feel free to close this PR if you are still unconvinced (until 3rd PR 😉 )

@ezolenko
Copy link
Owner

ezolenko commented Jul 13, 2018

Does recent es-lint infection change anything either way?

On one hand baked dependencies would prevent things in my dependency chain from changing in unexpected ways, on the other if something makes it in, it will stay there until this package is unpublished itself.

For bundled package to stay safe I will need to get lucky and not be infected at the time of publishing. For unbundled package, users will have to be lucky every time they do npm install. So more luck required in total, but that also can be fixed dynamically.

@agilgur5
Copy link
Collaborator

I was considering filing a PR/issue for this as well until I saw this when searching if this was a mistake or intentional.

While I can understand some reasoning behind dependency trust issues, bundling dependencies for this one package doesn't really fix an ecosystem-wide issue:

  1. Users of this package probably use many other packages that don't bundle their dependencies. That's the standard / convention / expectation and this is the exception.
  2. When you update a dependency, how do you know it can be trusted? Do you check its source, dist, and then recursively for all its dependencies? Bundling doesn't change the trust issue at the core here.
  3. Bundling a dependency means security patches are non-trivial to make as well, and that users must wait for you to update a security patch instead of doing it themselves. Not to mention that because dependencies aren't listed as dependencies, users won't even be alerted of security issues during an audit.
    That reduces this package's security and is itself a trust issue. Security patches happen pretty frequently too, whereas trust issues happen rather infrequently (and are harder to fix as a result of this).

@ezolenko
Copy link
Owner

ezolenko commented Feb 11, 2020

@agilgur5

1 and 3 are good points.

Re 2 I actually check dependency updates -- because they are bundled up, rollup throws away anything not used (which is most of the stuff) and diff for bundled source is reasonable. I haven't reviewed the whole source of all packages when I originally included them, but I did review every change that actually makes it into the bundle after that. Can't say how effective that review is, I'm basically making sure changes are reasonably looking and no fishy http requests and such, but such review won't be possible at all if dependencies are not bundled.

For example check out differences here after a couple of dependent packages were updated:
0420a5f#diff-3e1f21880c09d0f007b95dbf39685ae1

Or here:
3d3e4eb#diff-3e1f21880c09d0f007b95dbf39685ae1

@ezolenko
Copy link
Owner

ezolenko commented Feb 11, 2020

That said, I wouldn't mind a PR for a separate package with unbundled dependencies. I can publish them both for a while, and if most of usage switches to it, I can deprecate original one.

@agilgur5
Copy link
Collaborator

agilgur5 commented Oct 9, 2020

That said, I wouldn't mind a PR for a separate package with unbundled dependencies. I can publish them both for a while, and if most of usage switches to it, I can deprecate original one.

While the intent of an "A/B test" like that sounds useful, realistically that's not actually an A/B test as it requires very deliberate opt-in. And the benefits aren't obvious either. Would need a pretty big, visible mention in the docs, which I think would be confusing and if this change is considered "controversial", so would that then.
TSDX would switch immediately which is like ~12% of the usage. I could ask microbundle (~8%) and microbundle-crl (~3%) to do similar things, getting to ~25% switch pretty quickly.
But I think that ruins the point -- that's not really testing anything or doing much meaningful at that point. Which is why I'm not sure what purpose all the extra work for a second package serves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants