gajus / table

Formats data into a string table.
Other
899 stars 77 forks source link

RFC: Build using rollup.js #129

Closed realityking closed 3 years ago

realityking commented 3 years ago

This PR uses rollup.js to bundle all table into a single (CommonJS) JavaScript file.

This has a few small advantages:

More excitingly, it would open a path to making ajv a pure dev dependency with an approach like this one: 0d3fe98b83d0da15d9ae09ccd3851f9e30f78db9 (I didn't make it part of this PR as I think it warrants its own review).

There's two things I changed that I'm not sure of:

Both can easily be changed if desired.

This is quite an invasive change to how table is bundled so I fully understand if you don't wanna go down this path. Let me know what you think.

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 223


Totals Coverage Status
Change from base Build 220: 6.4%
Covered Lines: 744
Relevant Lines: 892

💛 - Coveralls
gajus commented 3 years ago

It is always a good idea to raise an issue before raising a PR. Otherwise, it doesn't feel nice to just shutdown good PRs, and it definitely does not make someone to contribute again.

In this particular case, I don't see the benefits named (bundle size) as being of great benefit considering this package is for Node.js consumption. The require time does not change in any meaningful way.

realityking commented 3 years ago

Please don’t worry about deterring me. I know this kinda PR is a significant change and not always something the maintainer wants to consider.

I agree with you on the bundle size but have you considered the gain of removing ajv as a dependency that I mentioned? You can find an implementation in this commit: 0d3fe98

gajus commented 3 years ago

The bigger question is "why".

If this was a package that was ever included in browser, I'd be Okay with it.

But since it is only ever used in CLI, and since (when gzipped) all the JavaScript files are tiny, it makes little to no difference to anyone who consumes the package.

I generally follow the KISS principle until someone can demonstrate something that is either visibly breaking or limits them from using the package.

In this particular case, the only use case for rollup that I can see would be to make this package compatible with Deno. I am not an expert on Deno, though.

However, all of this would come at a cost of debug-ability + the overall size of your dependency tree would (very likely) be greater after these changes, because you are bundling dependencies that can no longer be deduplicated.

realityking commented 3 years ago

I've no experience with Deno so I can't comment on that part.

I do think there's an inherent value in reducing the number of dependencies of packages where possible. Remembering the whole ajv/eslint story from a few years ago is a good example of why. Another reason is to simply reduce the surface of packages that could trip vulnerability scanners. It also helps deduplicate packages which can help improve performance since the same module isn't loaded multiple times.

Now the maintenance trade-off is obviously to consider. I'm very familiar with rollup so for me it wouldn't be much of a burden. It might be different for you so I also understand if this is not something you desire for your package.

the overall size of your dependency tree would (very likely) be greater after these changes, because you are bundling dependencies that can no longer be deduplicated

I don't follow what you mean here. The only file that'd be bundled into table would be this one:

https://github.com/ajv-validator/ajv/blob/fe591439f34e24030f69df9eb8d91e6d037a3af7/lib/compile/equal.js#L1-L5

It simply aliases to fast-deep-equal. This changes pulls that alias into table and make fast-deep-equal a direct dependency. ajv itself and all its other dependencies would become dev dependency.