Skip to content

Include manifestURI in manifest object#629

Merged
yknl merged 2 commits intostx-labs:developfrom
friedger:patch-4
Apr 30, 2019
Merged

Include manifestURI in manifest object#629
yknl merged 2 commits intostx-labs:developfrom
friedger:patch-4

Conversation

@friedger
Copy link
Copy Markdown
Contributor

@friedger friedger commented Mar 18, 2019

This PR

  • adds property manifestURI to the loaded manifest object

Applications want to know where the manifest came from, for example stacks-archive/blockstack-browser#1783

@codecov-io
Copy link
Copy Markdown

codecov-io commented Mar 18, 2019 •
edited by codecov bot
Loading

Codecov Report

Merging #629 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #629   +/-   ##
========================================
  Coverage    75.86%   75.86%           
========================================
  Files           56       56           
  Lines         2780     2780           
  Branches       219      219           
========================================
  Hits          2109     2109           
  Misses         642      642           
  Partials        29       29
Impacted Files Coverage Δ
src/auth/authProvider.ts 48.38% <100%> (ø) ⬆️

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 8396aad...d36a177. Read the comment docs.

@zone117x zone117x self-requested a review March 18, 2019 12:57
@friedger friedger mentioned this pull request Mar 18, 2019
Closed
@yknl
Copy link
Copy Markdown
Contributor

yknl commented Mar 20, 2019

I'm not totally understanding the use case for this, the application has the ability to specify the manifestURI through redirectToSignIn().

@friedger
Copy link
Copy Markdown
Contributor Author

friedger commented Mar 20, 2019 •
edited
Loading

@yknl when I ask blockstack.js to decode a jwt and fetch the manifest I do not know where the manifest came from. Issue 1875 of the browser is my use case

@yknl
Copy link
Copy Markdown
Contributor

yknl commented Mar 20, 2019

Ok got it, this is so that the Blockstack browser can handle relative app icon URLs not for applications.

@friedger
Copy link
Copy Markdown
Contributor Author

friedger commented Mar 20, 2019 via email

@yknl
Copy link
Copy Markdown
Contributor

yknl commented Mar 20, 2019

That's true 👍

@zone117x
Copy link
Copy Markdown
Contributor

zone117x commented Apr 24, 2019

@friedger can you merge develop into this PR?

@friedger
Copy link
Copy Markdown
Contributor Author

friedger commented Apr 25, 2019

@zone117x Done :-)

@zone117x zone117x requested a review from yknl April 25, 2019 15:26
zone117x
zone117x approved these changes Apr 25, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@zone117x zone117x left a comment

Choose a reason for hiding this comment

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

LGTM

@yknl yknl merged commit 2dd7540 into stx-labs:develop Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@zone117x zone117x zone117x approved these changes

@yknl yknl yknl approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@friedger @codecov-io @yknl @zone117x