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(deps): adds external context dep #151

Merged
merged 2 commits into from
May 12, 2020

Conversation

andrewxhill
Copy link
Member

No description provided.

Signed-off-by: andrew <andrew@textile.io>
@andrewxhill
Copy link
Member Author

okay, first error

  1) Users...
       getThread
         should handle bad keys:

      AssertionError: expected 5 to equal 16
      + expected - actual

      -5
      +16
      
      at /home/runner/work/js-textile/js-textile/src/users.spec.ts:51:29
      at Generator.throw (<anonymous>)
      at rejected (src/users.spec.ts:6:65)
      at processTicksAndRejections (internal/process/task_queues.js:97:5)

The actual error coming back is 'Thread not found'. which is strange since you'd think auth would come before exists

@andrewxhill
Copy link
Member Author

The second issue

  2) Users...
       getThread
         should handle users keys:

is erroring out because it's not throwing an error when the token doesn't exist

@carsonfarmer
Copy link
Member

Hmmm, so now that Context is mutable, its possible that we are properly authenticated, and it needs to be cleared out?

@andrewxhill
Copy link
Member Author

Okay, same errors with 3 and 4. will try to scope the context a bit more

@andrewxhill
Copy link
Member Author

what do you make of error 1 though?

@carsonfarmer
Copy link
Member

taking a look now

Signed-off-by: Carson Farmer <carson.farmer@gmail.com>
@carsonfarmer
Copy link
Member

The issue was indeed that the Context was being mutated underneath us due to the fact that it is now mutable. The mutable API is easier to hide, but harder to deal with. Its a trade off I guess? Anyway, I have also tried to reduce the number of times we actually reference the context in the tests to hopefully test how well we can 'hide' it. Also, this unexpected mutation is really only an issue for tests, where we want to throw errors etc.

@andrewxhill andrewxhill added this to the Sprint 36 milestone May 12, 2020
@andrewxhill andrewxhill merged commit 2d04be6 into master May 12, 2020
@andrewxhill andrewxhill deleted the andrew/update-new-context-api2 branch May 12, 2020 18:49
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.

2 participants