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

Support new import syntax through config.load #1511

Conversation

justinbarry
Copy link
Contributor

@justinbarry justinbarry commented Dec 12, 2022

Addresses: #1514

@justinbarry justinbarry requested a review from a team as a code owner December 12, 2022 21:35
@changeset-bot
Copy link

changeset-bot bot commented Dec 12, 2022

🦋 Changeset detected

Latest commit: 58656c8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@onflow/config Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@justinbarry justinbarry force-pushed the GH-1486-Support-new-flowjson-and-import-syntax-through-configload branch 2 times, most recently from 81f4cff to 1dd5725 Compare December 12, 2022 21:45
Copy link
Member

@chasefleming chasefleming left a comment

Choose a reason for hiding this comment

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

Lots of good stuff here, but a few missing pieces still I think.

  • I don't think we should use placeholders (e.g. 0xHelloWorld) for storing v2 contract aliases in config. We want to get away from them so something more hidden like lib.contract.0xHelloWorld or something I think would be better. Less change people will use them then. And then flow v1 syntax won't occasionally work with v2 causing confusion.
  • I don't actually see the handling of the new import syntax mentioned in the CLI issue. That would be handled in resolve-cadence.js. Think that needs to get added.
  • I think the v2 json is slightly incorrect, but could be wrong. cc'd @sideninja on that.

Also, can you link a ticket and update the title to the type, package, description format please?

packages/config/src/config.js Outdated Show resolved Hide resolved
packages/config/src/config.test.js Outdated Show resolved Hide resolved
packages/config/src/config.test.js Show resolved Hide resolved
packages/config/utils/utils.js Show resolved Hide resolved
packages/config/utils/utils.js Outdated Show resolved Hide resolved
packages/config/utils/utils.js Outdated Show resolved Hide resolved
packages/config/utils/utils.test.js Outdated Show resolved Hide resolved
packages/config/src/config.test.js Outdated Show resolved Hide resolved
packages/config/src/config.test.js Outdated Show resolved Hide resolved
packages/config/src/config.test.js Outdated Show resolved Hide resolved
@justinbarry justinbarry marked this pull request as draft January 10, 2023 19:46
@justinbarry justinbarry force-pushed the GH-1486-Support-new-flowjson-and-import-syntax-through-configload branch 2 times, most recently from 9b8b398 to ac939cb Compare January 28, 2023 00:48
@justinbarry justinbarry requested a review from sideninja January 28, 2023 00:52
@justinbarry justinbarry force-pushed the GH-1486-Support-new-flowjson-and-import-syntax-through-configload branch from ac939cb to e907bf3 Compare January 28, 2023 00:53
@justinbarry justinbarry marked this pull request as ready for review January 28, 2023 00:53
@sideninja
Copy link
Contributor

Looks good, left couple of comments, I think it's good idea to add another test proving the resolution of address

packages/config/src/config.js Outdated Show resolved Hide resolved
packages/config/utils/utils.js Outdated Show resolved Hide resolved
packages/sdk/src/resolve/resolve-cadence.js Outdated Show resolved Hide resolved
packages/sdk/src/resolve/resolve-cadence.js Outdated Show resolved Hide resolved
@chasefleming
Copy link
Member

I just found a bug in the load. It wasn't created in this PR, but can we fix it in it? The bug is that it still warns about private keys if you use a CLI env var like $FLOW_EMULATOR_PRIVATE_KEY instead of the from path. Can we also just make it not flag if you're using the $ method for env vars from CLI

@fotescodev
Copy link

@justinbarry sir! We just got another good piece of feedback on flow dev / flow setup commands, and it's dependent on the FCL's support of the new JSON Schema: onflow/flow-cli#826

How can we move forward with this PR?

@justinbarry justinbarry linked an issue Feb 3, 2023 that may be closed by this pull request
@justinbarry justinbarry changed the title Gh 1486 support new flowjson and import syntax through configload Support new import syntax through config.load Feb 3, 2023
@justinbarry
Copy link
Contributor Author

I just found a bug in the load. It wasn't created in this PR, but can we fix it in it? The bug is that it still warns about private keys if you use a CLI env var like $FLOW_EMULATOR_PRIVATE_KEY instead of the from path. Can we also just make it not flag if you're using the $ method for env vars from CLI

Let's make that another issue so I can get this out for testing. My hope was we would get this into master ready to release, but I don't think it is going to work out that way.

@justinbarry justinbarry force-pushed the GH-1486-Support-new-flowjson-and-import-syntax-through-configload branch from e907bf3 to 6c9d312 Compare February 7, 2023 00:06
@justinbarry
Copy link
Contributor Author

justinbarry commented Feb 7, 2023

Given Flow.json V2 is yet to be out in the wild, I ripped out all the references to it, and we can add them back later at a future date. The only outstanding question is what happens when a contract in flow.json has both an alias and a deployment.... which one wine? @sideninja Let's talk about this in the morning. Otherwise, this should be good to go.

@justinbarry justinbarry force-pushed the GH-1486-Support-new-flowjson-and-import-syntax-through-configload branch 2 times, most recently from 76d283d to 7edc6ce Compare February 7, 2023 00:31
@sideninja
Copy link
Contributor

sideninja commented Feb 7, 2023

@justinbarry
It shouldn't have it, but even it does always prioritize the alias. Ofc it can have deployments and alias in case of a specific network. So you can imagine it has an alias for testnet but it's also in deployments because on emulator the deployments is used since there's no alias for emulator. So for the network you are using if the alias exists use that, otherwise you can use the deployment.

@justinbarry justinbarry force-pushed the GH-1486-Support-new-flowjson-and-import-syntax-through-configload branch from 7edc6ce to 5388407 Compare February 7, 2023 19:37
@justinbarry justinbarry force-pushed the GH-1486-Support-new-flowjson-and-import-syntax-through-configload branch from 5388407 to e856f1c Compare February 8, 2023 17:27
@justinbarry justinbarry force-pushed the GH-1486-Support-new-flowjson-and-import-syntax-through-configload branch from e856f1c to 58656c8 Compare February 9, 2023 18:12
@justinbarry justinbarry merged commit d35a777 into master Feb 9, 2023
@justinbarry justinbarry deleted the GH-1486-Support-new-flowjson-and-import-syntax-through-configload branch February 9, 2023 18:30
@github-actions github-actions bot mentioned this pull request Jun 2, 2023
@jribbink jribbink mentioned this pull request Jan 10, 2024
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.

Support import syntax through config.load
4 participants