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

Browser support #265

Closed
wants to merge 3 commits into from
Closed

Browser support #265

wants to merge 3 commits into from

Conversation

jamesramsay
Copy link
Owner

Requires a custom resolveLink function to be provided which is responsible for handling link inflation.

@jamesramsay
Copy link
Owner Author

jamesramsay commented Jun 11, 2016

@jmatsushita please could you take a look at this PR and see if it fits the bill?

I think a solution like this would solve #209, #218, and #259.

@codecov-io
Copy link

codecov-io commented Jun 11, 2016

Current coverage is 99.30% (diff: 100%)

Merging #265 into master will increase coverage by 0.05%

@@             master       #265   diff @@
==========================================
  Files            12         12          
  Lines           266        288    +22   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            264        286    +22   
  Misses            2          2          
  Partials          0          0          

Powered by Codecov. Last update 712a6cf...e7f9f42

@Ramblurr
Copy link

Could you document the resolveLink option in the README?

@Ramblurr
Copy link

Ramblurr commented Jul 12, 2016

Also, for non-browser implementations, it would be great to be able to rely on the default implementation of linkResolver.

My use case is where I want to be able to handle some custom links, but also support the base feature of including files via paths.

It doesn't seem possible to get a handle on the exported resolveLink function in resolve.js. Either adding it to the public API in hercule.js or passing it as an option to options.resolveLink would be nice.

@jamesramsay
Copy link
Owner Author

@Ramblurr I'll absolutely add documentation before the branch merges, just haven't had a chance yet.

In this first version I hadn't really planned on supporting extending the base implementation, but perhaps I should reevaluate.

@Ramblurr
Copy link

@jamesramsay Cool!

Another piece of feedback, it doesn't seem to work with nested transcludes.

I wrote a simple customLinkResolver that calls the default implementation in resolve.js, but my version is only passed the transcludes in the base file. The nested transcludes are never passed to the custom resolver.

Is this a bug or intended?

@jamesramsay
Copy link
Owner Author

@Ramblurr that's a bug. I'll be sure to add a test for that 😉

@Ramblurr
Copy link

Awesome! I am stoked for this feature, it will really allow for some great opportunities (Such as #270)

Another possibility is some short-hand notation for api blueprint projects.

For example we include our JSON body's with a transclude :[a task](../../models/task.json), but it is cumbersome to remember the number of ../ you need. With the custom link resolver, we can shorten this to simply :[a task](models/task)

But yea, having nested transcludes working is a must.

BTW, I simply added export { resolveLink } from './resolve' to hercule.js to expose the default resolveLink implementation to clients.

@Ramblurr
Copy link

Any updates on this issue? We're using our own forked hercule right now because we need access to the link resolver. Note that we aren't even using hercule in the browser, we just wanted to write our own extensions to the transclude syntax to support shortcuts and other processing.

@jamesramsay
Copy link
Owner Author

@Ramblurr sorry for the lack of updates. It had really fallen off my radar. I'll try to dust this off and publish an MVP without browser considerations in the near future.

@jamesramsay jamesramsay changed the base branch from master to master-3.x October 31, 2016 22:34
@jamesramsay
Copy link
Owner Author

@Ramblurr I split out the portion not related to browser support and released it in v3.1.0. I haven't added documentation yet because I want to clean up the API make it easier to use first.

@jamesramsay
Copy link
Owner Author

@Ramblurr I realised I never left a comment to let you know hercule 4.x has a documented API for custom resolvers. Feel free to open an issue if you have any questions!

@jamesramsay
Copy link
Owner Author

Closing this PR due to age. Latest release could be used in browser with a custom resolve.

@jamesramsay jamesramsay deleted the browser-safe branch October 30, 2018 13:55
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