-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add binding gen js for remote and local #12
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments. Also, it does feel like the bindings stuff is going to need to be in its own internal package.
…late name to be fcl specific
return t.Data.Type == "transaction" | ||
} | ||
|
||
func (t *FlowInteractionTemplate) GetAndReplaceCadenceImports(networkName string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is for executing the flix. Good catch we should have this "import" syntax in a helper lib that all tools import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok. Well then it might be ok. Since we kind of control how imports look like in a flix.
How will we know these templates generated are compatible with the user's installed version of FCL? What if the interface of FCL changes in FCL 2.0, 3.0, etc? Or, what if later version flixkit-go leverages new FCL features in the future, but the user is using an older version of FCL? |
Good point and good catch. It'll get a little ugly to use I'll add some verbiage in the comment block of the generated javascript that indicates they need version 1.3.0 or greater. |
In FCL we try to "normalize" the interaction template before its used, and in doing so we check that the version of the interaction template is compatible with that version of FCL: So we do have some safeguards today around versioning. That said, we may want to think up a more elegant solution that doesn't require us to release a new version of FCL when a new version of Interaction Template is released. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bthaile It's a bit of a black box right now unless you know the tools well. Even for me with some context it takes a little to figure out what is happening. This is a new feature and should require docs added. Can you create a page explaining this, how to use it, and what a library needs to do in order to add their library? Then can you link that page off the repo under a new section?
for contractName, networks := range contracts { | ||
network, ok := networks[networkName] | ||
if !ok { | ||
return "", fmt.Errorf("network %s not found for contract %s", networkName, contractName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the empty string returned if not found? won't that fail later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A error is returned, I'd prefer to return a nil. Since I just moved the code, I didn't want to modify it.
pattern := fmt.Sprintf(`import\s*%s\s*from\s*%s`, contractName, dependencyAddress) | ||
re, err := regexp.Compile(pattern) | ||
if err != nil { | ||
return "", fmt.Errorf("invalid regex pattern: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same response, A error is returned, I'd prefer to return a nil. Since I just moved the code, I didn't want to modify it.
IsLocalTemplate bool | ||
} | ||
|
||
type FclJSGenerator struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be lowercased?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's publicly assessable. I thought it needed to be uppercase so calls can get to ti.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It has to be uppercased for other packages to access it yes.
I added usage verbiage in readme, it ended up being really simple. |
Added FileReader to config for reading files locally, so it can be mocked out for testing.
Moved shared objects to
common
moduleAdded support for js docs
With flix version 1.1 there'll be a return type that can be added to js docs for scripts.