feat: add multi explorer support#241
Conversation
|
|
One of the challenges here is that not all explorers have the same API for selecting different clusters. So while the examples in your tests may work for the Most notably solana.fm breaks the contract here: This works: I've suggested to fix this numerous times but solana.fm doesn't care about it. |
|
@beeman is absolutely right. This was one of the reasons that I did not add other explorers originally |
|
Ah okay - it seems like solana.fm uses |
|
That's definitely an option, either way some explorers need to have custom config for different scenarios. But that also bloats the function to handle these one offs. Most devs will always use the same explorer and not the others. Part of me thinks it's a better idea to just have a different function for each explorer, but that also duplicates codes |
|
In the previous iterations of the create-solana-dapp templates we shipped a However, I think having different functions for each explorer might be the best solution here. It's trivial to implement another one, and anyone will only bundle in only one of these functions. So independent of how many explorers are supported, any person will only pay the price for 1. Having '1 function do 1 thing' also seems in line with the rest of Gill and Kit. I think something like this is might be good: function getExplorerLink() {}
function getExplorerLinkOrb() {}
function getExplorerLinkSolanafm() {}
function getExplorerLinkSolscan() {} |
|
Yeah, I'm game for this |
|
Should be good for review now @nickfrosty @beeman |
|
I have a solution here that works a bit more abstractly so we don't have to keep supporting a ton of explorers manually in this library, let me know if you have thoughts |
@macalinao god no. there is no reason for all that complexity to create a link. |
GuiBibeau
left a comment
There was a problem hiding this comment.
This is looking clean! I'd like to see a few things before this gets merged:
- add to the JsDocs comments the parameters of the functions
/**
* Description of the function.
* @param {Type} parameterName - Description of the parameter.
*/
- look into if moving a few stuff to constants since they are repeated in many places in the code. I know these URLs likely won't change much in the future due to the nature of an explorer but it's an easy win and good practice.
I will leave an approval since i don't think any of those two comments warrant further back and forth but i'd really appreciate if we do them before merging given they take 5 minutes to do.
Problem
gill only supports Solana Explorer right now. It's good to allow flexibility according to dev's personal preference of explorer.
Summary of Changes
Add a new optional parameter in
getExplorerLinkwhere user can specify the explorer they want to use for the link, defaults to Solana Explorer.Fixes #
Solves #206