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

Refactor #94

Merged
merged 20 commits into from Feb 6, 2017

Conversation

Projects
None yet
3 participants
@thomasconner
Contributor

thomasconner commented Jan 10, 2017

Description

This PR updates several parts of the SDK to allow further development.

Changes

  • Export all modules so they are easily extensible by shims.
  • Create aliases using babel to prevent long ../../../ expressions when importing modules
  • Expose the rack architecture to allow shims to provide custom implmentations for network and cache middleware
  • Clean some minor things with the way modules are exported

thomasconner added some commits Jan 4, 2017

Move files
Move src directory into src/core. Create network and cache directories.
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jan 23, 2017

Codecov Report

Merging #94 into master will increase coverage by -0.96%.

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
- Coverage    74.5%   73.54%   -0.96%     
==========================================
  Files          75       72       -3     
  Lines        6687     6679       -8     
  Branches     1063     1055       -8     
==========================================
- Hits         4982     4912      -70     
- Misses       1705     1767      +62
Impacted Files Coverage Δ
src/errors/src/insufficientCredentials.js 100% <ø> (ø)
src/errors/src/invalidCredentials.js 100% <ø> (ø)
src/datastore/src/datastore.js 58.33% <100%> (-1.09%)
src/endpoint.js 100% <100%> (ø)
src/request/src/middleware/src/cache.js 100% <100%> (ø)
src/request/src/headers.js 83.83% <100%> (ø)
src/request/index.js 100% <100%> (ø)
src/kinvey.js 98.01% <100%> (+0.09%)
src/datastore/src/sync.js 85.41% <100%> (+0.38%)
src/entity/src/user.js 48.19% <100%> (-0.14%)
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abe1e30...738d18e. Read the comment docs.

codecov-io commented Jan 23, 2017

Codecov Report

Merging #94 into master will increase coverage by -0.96%.

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
- Coverage    74.5%   73.54%   -0.96%     
==========================================
  Files          75       72       -3     
  Lines        6687     6679       -8     
  Branches     1063     1055       -8     
==========================================
- Hits         4982     4912      -70     
- Misses       1705     1767      +62
Impacted Files Coverage Δ
src/errors/src/insufficientCredentials.js 100% <ø> (ø)
src/errors/src/invalidCredentials.js 100% <ø> (ø)
src/datastore/src/datastore.js 58.33% <100%> (-1.09%)
src/endpoint.js 100% <100%> (ø)
src/request/src/middleware/src/cache.js 100% <100%> (ø)
src/request/src/headers.js 83.83% <100%> (ø)
src/request/index.js 100% <100%> (ø)
src/kinvey.js 98.01% <100%> (+0.09%)
src/datastore/src/sync.js 85.41% <100%> (+0.38%)
src/entity/src/user.js 48.19% <100%> (-0.14%)
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update abe1e30...738d18e. Read the comment docs.

@tejasranade

Few comments, mostly minor.

Show outdated Hide outdated src/request/src/rack.js Outdated
"loglevel": "^1.4.1",
"promise-queue": "^2.2.3",
"qs": "^6.2.0",
"append-query": "1.1.0",

This comment has been minimized.

@tejasranade

tejasranade Jan 23, 2017

Member

@thomasconner what's the reason to require specific versions here?

@tejasranade

tejasranade Jan 23, 2017

Member

@thomasconner what's the reason to require specific versions here?

This comment has been minimized.

@thomasconner

thomasconner Jan 23, 2017

Contributor

We should be installing specific versions. For now we haven't had an issue but we are risking the chance that things could not work if a new version is released for a lib we are using.

@thomasconner

thomasconner Jan 23, 2017

Contributor

We should be installing specific versions. For now we haven't had an issue but we are risking the chance that things could not work if a new version is released for a lib we are using.

CustomEndpoint,
DataStore,
DataStoreType,
FileStore as File,
DeltaFetchRequest,
FileStore as Files,

This comment has been minimized.

@tejasranade

tejasranade Jan 23, 2017

Member

@thomasconner Makes sense to remove the File export. Have you checked that all docs and references are correct and do not mention using File anywhere?

@tejasranade

tejasranade Jan 23, 2017

Member

@thomasconner Makes sense to remove the File export. Have you checked that all docs and references are correct and do not mention using File anywhere?

This comment has been minimized.

@thomasconner

thomasconner Jan 23, 2017

Contributor

Yes I have. We have been using Kinvey.Files in the docs for sometime now.

@thomasconner

thomasconner Jan 23, 2017

Contributor

Yes I have. We have been using Kinvey.Files in the docs for sometime now.

followRedirect: followRedirect,
timeout: timeout
}, (error, response, body) => {
if (isDefined(response) === false) {

This comment has been minimized.

@tejasranade

tejasranade Jan 23, 2017

Member

@thomasconner this changes some behavior

  • on timeout errors
  • value of this.httpRequest at the end of the request
  • redirects has changed

Are these intentional? Is there test coverage on these?

@tejasranade

tejasranade Jan 23, 2017

Member

@thomasconner this changes some behavior

  • on timeout errors
  • value of this.httpRequest at the end of the request
  • redirects has changed

Are these intentional? Is there test coverage on these?

This comment has been minimized.

@thomasconner

thomasconner Jan 30, 2017

Contributor

@tejasranade I added some additional unit tests to cover different kinds of requests.

@thomasconner

thomasconner Jan 30, 2017

Contributor

@tejasranade I added some additional unit tests to cover different kinds of requests.

@@ -1,4 +1,4 @@
import { Client } from 'src/client';
import Client from 'src/client';

This comment has been minimized.

@tejasranade

tejasranade Jan 23, 2017

Member

@thomasconner Does this import change impact apps?

@tejasranade

tejasranade Jan 23, 2017

Member

@thomasconner Does this import change impact apps?

This comment has been minimized.

@thomasconner

thomasconner Jan 23, 2017

Contributor

No it won't. Apps would access the Client on the Kinvey namespace.

@thomasconner

thomasconner Jan 23, 2017

Contributor

No it won't. Apps would access the Client on the Kinvey namespace.

thomasconner added some commits Jan 23, 2017

Merge branch 'master' into refactor
* master:
  3.3.5
  Fix failing unit test
  Update the CHANGELOG for v3.3.5

thomasconner added some commits Feb 1, 2017

MLIBZ-1576 Fix acl constructor errors
Don't default entity argument to an empty object. Create an _acl property if the provided entity argument does not contain an _acl property.
@thomasconner

This comment has been minimized.

Show comment
Hide comment
@thomasconner

thomasconner Feb 1, 2017

Contributor

@tejasranade can you review the changes you requested and see if they satisfy your review?

Contributor

thomasconner commented Feb 1, 2017

@tejasranade can you review the changes you requested and see if they satisfy your review?

@tejasranade

LGTM

Merge pull request #103 from Kinvey/MLIBZ-1576_ACL_Error
MLIBZ-1576: Kinvey V3 add acl to individual entity error

@thomasconner thomasconner merged commit 0512504 into master Feb 6, 2017

0 of 2 checks passed

codeclimate Code Climate is analyzing this code.
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@thomasconner thomasconner deleted the refactor branch Feb 6, 2017

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