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: use $ReadOnlyArray when input array is not modified (fixes #84) #100

Merged
merged 7 commits into from
Aug 22, 2017

Conversation

gajus
Copy link
Contributor

@gajus gajus commented Aug 21, 2017

No description provided.

@gajus
Copy link
Contributor Author

gajus commented Aug 21, 2017

As far as I can tell, these are the only safe changes.

I have left output type as Array as that would be a breaking change/ outside of scope.

I have left comments as Array since $ReadOnlyArray is a sub-type of Array.

cc @wincent

@gajus
Copy link
Contributor Author

gajus commented Aug 21, 2017

The build process is failing because of:

/home/travis/build/facebook/dataloader/src/index.js
   13:1   error  Line 13 exceeds the maximum line length of 80  max-len
   13:33  error  '$ReadOnlyArray' is not defined                no-undef
   13:63  error  '$ReadOnlyArray' is not defined                no-undef
  133:18  error  '$ReadOnlyArray' is not defined                no-undef
  309:26  error  '$ReadOnlyArray' is not defined                no-undef

Have you considered using https://github.com/gajus/eslint-plugin-flowtype?

I have added $ReadOnlyArray to globals as an interim fix.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 8ce9138 on gajus:feat/use-readonlyarray into 75a0dfb on facebook:master.

This was added in 610b4bf but I think the need for it was
obviated by the subsequent Flow upgrade in f0711ec.
@wincent
Copy link
Contributor

wincent commented Aug 21, 2017

Thanks @gajus. Will merge with a couple of tweaks applied on top.

@gajus
Copy link
Contributor Author

gajus commented Aug 22, 2017

Regarding feee2aa, that was an ESLint error, not Flow.

@@ -162,7 +162,7 @@ export default class DataLoader<K, V> {
}

/**
* Adds the provided key and value to the cache. If the key already exists, no
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea how this got into the PR. Sorry.

@wincent
Copy link
Contributor

wincent commented Aug 22, 2017

Regarding feee2aa, that was an ESLint error, not Flow.

Bah, should have waited for Travis...

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 8a8651a on gajus:feat/use-readonlyarray into 75a0dfb on facebook:master.

@wincent wincent merged commit d19468f into graphql:master Aug 22, 2017
@gajus gajus mentioned this pull request Nov 8, 2017
@gajus
Copy link
Contributor Author

gajus commented Nov 10, 2017

@wincent Can this be released please?

I am still getting dozens of:

Error: src/factories/createLoaders.js:267
267:     ReservationByReservationUidLoader: new DataLoader(getReservationByReservationUids),
                                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ function. This type is incompatible with the expected param type of
 45:     batchLoadFn: BatchLoadFn<K, V>,
                      ^^^^^^^^^^^^^^^^^ function type. See: node_modules/dataloader/index.js.flow:45
  This parameter is incompatible:
    143:     return getByIdsLoader(connection, 'reservation', reservationUids, 'uid');
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type application of identifier `Promise`. Has some incompatible type argument with
     13: type BatchLoadFn<K, V> = (keys: Array<K>) => Promise<Array<V | Error>>;
                                                      ^^^^^^^^^^^^^^^^^^^^^^^^^ type application of identifier `Promise`. See: node_modules/dataloader/index.js.flow:13
      Type argument `R` is incompatible:
         42: ): Promise<$ReadOnlyArray<QueryResultRowType>> => {
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ read-only array type. This type is incompatible with. See: src/utilities/getByIdsLoader.js:42
         13: type BatchLoadFn<K, V> = (keys: Array<K>) => Promise<Array<V | Error>>;
                                                                  ^^^^^^^^^^^^^^^^ array type. See: node_modules/dataloader/index.js.flow:13

It is really discouraging to contribute anything if it does not get released for months.

@gajus
Copy link
Contributor Author

gajus commented Nov 15, 2017

@leebyron Any comment here?

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

Successfully merging this pull request may close these issues.

3 participants