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

chore: replace @ungap/from-entries with own helper #3491

Closed
wants to merge 1 commit into from

Conversation

netlify-team-account-1
Copy link
Contributor

- Summary

In the spirit of #3445, I took a look at our package.json to identify other modules we could likely bring into the repository to reduce installation time (which correlates more with number of packages, that with their individual size). @ungap/from-entries is one of them: It's a polyfill for Object.fromEntries, meaning a naive version can easily be rebuilt oneself. This PR introduces a naive replacement.
Bringing it into the repository, however, brings a bigger maintance burden (writing tests, catching edge cases).

It's a good thing that there are specialised packages for this - but since npm install doesn't scale well with number of packages installed, it has a negative impact on CLI's install time.

In order to bring down install time, we could think about bundling together oft-used modules in a netlify/utils package. Other candidates for that would be @sindresorhus/slugify and is-plain-obj, both of which are used across CLI, Build, and Config. By bundling these dependencies at build-time of netlify/utils (aka: shipping them as part of the own dist files, don#t include them in dependencies), we could bring down number of packages installed somewhat.

OTOH, Netlify-controlled dependencies are most likely a drop in the sea, as long as packages that we don't want to bundle bring in these helper packages. So we likely won't see a noteworthy improvement from introducing netlify/utils.

(this PR is not meant to be merged, but as an argument to the read-pkg-up debate)

- Test plan

- A picture of a cute animal (not mandatory but encouraged)

@netlify-team-account-1 netlify-team-account-1 added the type: chore work needed to keep the product and development running smoothly label Oct 13, 2021
@github-actions
Copy link

📊 Benchmark results

Comparing with 6d6022e

Package size: 353 MB

(no change)

^  352 MB  352 MB  352 MB  352 MB  352 MB  352 MB  352 MB  352 MB  352 MB  352 MB  352 MB  352 MB  353 MB 
│   ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T    
Legend

@ehmicky
Copy link
Contributor

ehmicky commented Oct 13, 2021

I am not convinced using our own small modules instead of re-using others would be an improvement, for several reasons:

  • This might not always reduce our package's size, due to deduping. It might actually be the opposite. Especially for sindresorhus' packages, which are most likely used by our other dependencies.
  • Bundling/vendoring might end up increasing the package's size, since different modules would not be able to share code. For example, netlify-cli uses @netlify/build which itself uses @netlify/zip-it-and-ship-it, but they all use some common utilities. Those should be in node_modules so they can be deduped.
  • Even small modules come with lots of niceties which we would need to maintain otherwise, especially automated tests and handling of specific edge cases. It is not uncommon to hit production bugs due to subtle issues in utilities.
  • Others' small modules are already typed with TypeScript, for many of them, which will help us transition to TypeScript.
  • The same applies to security: widely used small Node modules are triggering security notices (through GitHub, npm audit, etc.) when a vulnerability is found, while this would not apply if we had our own utilities.

Small installation size is good, but we should also consider maintenance cost, security and stability, which are more important IMHO

Overall, I don't think this would be an improvement and might distract us from other engineering initiatives. The installation size of netlify-cli is a problem, but we might want to address it by starting with removing big Node modules instead.

cc @erezrokah @eduardoboucas @JGAntunes What do you all think?

@erezrokah
Copy link
Contributor

cc @erezrokah @eduardoboucas @JGAntunes What do you all think?

I agree with @ehmicky. There's no data the suggested approach helps with installation size.
This PR specifically doesn't:
#3491 (comment)

Closing the PR. We can further discuss in an issue if we chose to.

@erezrokah erezrokah closed this Oct 19, 2021
@erezrokah erezrokah deleted the remove-from-entries branch October 19, 2021 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: chore work needed to keep the product and development running smoothly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants