Skip to content
This repository has been archived by the owner on Jul 6, 2020. It is now read-only.

Reduce size impact of populateExchange #122

Merged
merged 11 commits into from
Dec 8, 2019
Merged

Conversation

kitten
Copy link
Member

@kitten kitten commented Dec 3, 2019

  • Reduce size where possible of populateExchange by reusing more helpers we already had
  • Change some internal immutable clones to mutations mostly for micro-opt of perf)
  • Remove usage of Array.from for IE11 compat

Tests are still passing. 💯

Before Populate After Populate This PR
5.73kB 6.62kB 6.6kB

Refactor some parts to avoid unnecessary immutability for micro-perf optimisations,
and reduce size where possible by reusing helpers and swapping out immutability for
mutability.

Total minzipped size:
8.24kB -> 8.19kB
This isn't supported in IE11 and hence breaks compatibility
if it isn't user polyfilled.

See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/from
Copy link

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

Looking good, a few savings/perf boosts found

src/populateExchange.ts Outdated Show resolved Hide resolved
src/populateExchange.ts Outdated Show resolved Hide resolved
src/populateExchange.ts Outdated Show resolved Hide resolved
src/populateExchange.ts Show resolved Hide resolved
src/populateExchange.ts Outdated Show resolved Hide resolved
@kitten kitten force-pushed the chore/reduce-populate-size branch from a1e8efe to abfd97a Compare December 3, 2019 14:38
@kitten kitten marked this pull request as ready for review December 3, 2019 14:39
@kitten kitten requested a review from andyrichardson December 3, 2019 14:39
@kitten kitten force-pushed the chore/reduce-populate-size branch from 533ddaa to b76cce9 Compare December 3, 2019 15:13
userFragments,
}: AddFragmentsToQuery) => {
export const addFragmentsToQuery = (
schema: GraphQLSchema,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there an advantage to changing these to indexed parameters? It's less transparent as to what the arguments should be.

Copy link
Member Author

Choose a reason for hiding this comment

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

It gets rid of the property names in the output JS (although I'd love if Terser would do that automatically)

I personally think it's still transparent since TS typehints (of the TS language server for instance) still give you the signature with names and types.

src/types.ts Outdated Show resolved Hide resolved
src/ast/node.ts Show resolved Hide resolved
@kitten kitten force-pushed the chore/reduce-populate-size branch from 809e7a3 to cc633a0 Compare December 3, 2019 16:11
Copy link

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

Great work!

@kitten kitten merged commit 5607ee7 into master Dec 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants