Skip to content
This repository was archived by the owner on Jul 8, 2024. It is now read-only.

Conversation

@alxflw
Copy link
Contributor

@alxflw alxflw commented Jan 14, 2022

This PR fixes #47

All Cadence files that were inline are now in the one of two folders:

  • web/cadence/transactions/<name>.cdc
  • web/cadence/scripts/<name>.cdc

cc @shubik22

@psiemens
Copy link
Contributor

I think you can actually get away without using raw.macro.

I realized we already load Cadence files in the fcl-dev-wallet project, which is also a Next app. It works by adding this line in next.config.js: https://github.com/onflow/fcl-dev-wallet/blob/main/next.config.js#L23-L29

@alxflw
Copy link
Contributor Author

alxflw commented Jan 14, 2022

I think you can actually get away without using raw.macro.

I realized we already load Cadence files in the fcl-dev-wallet project, which is also a Next app. It works by adding this line in next.config.js: https://github.com/onflow/fcl-dev-wallet/blob/main/next.config.js#L23-L29

that worked! but it seems the cdc files have to be within the nextjs project root. LMK if that impacts testability

@alxflw alxflw changed the title [draft] feat: import cdc files feat: import cdc files Jan 14, 2022
@alxflw alxflw marked this pull request as ready for review January 14, 2022 01:59
Copy link
Contributor

@10thfloor 10thfloor left a comment

Choose a reason for hiding this comment

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

Great!

@10thfloor
Copy link
Contributor

Is there is a way to de-dupe the cadence files?

@psiemens
Copy link
Contributor

psiemens commented Jan 18, 2022

Is there is a way to de-dupe the cadence files?

Unfortunately Next requires that all loaded files are in the Next project root (/web in this case). That's one reason to copy the files into /web.

I came across this experimental feature to load external Typescript files: vercel/next.js#22867

Unfortunately it only seems to work for Typescript files and not arbitrary files loaded by Webpack.

It might be possible to load files from ../cadence using a custom Webpack config. I haven't tried this, though.

Lastly, if we do eventually convert this whole project to a Next app and remove /api, then we could move /web to the root and this problem goes away. 😄

The other problem is that the web version of the Cadence files use the 0xNonFungibleToken convention whereas the source Cadence files use file paths.

You can see here that the API replaces the file paths with addresses: https://github.com/onflow/kitty-items/blob/master/api/src/services/kitty-items.ts#L108-L123

@alxflw It'd be great to have some JS code that does this in a more generic way. It could be a nice utility that we use in both the API and web app.

@alxflw
Copy link
Contributor Author

alxflw commented Jan 18, 2022

thanks for the input y'all. I'll focus on two things:

  1. experiment with loading of files below root. I'll try the webpack config approach and see if I can get something working

  2. create a helper function and unify the importing style (aliases that are replaced with relative paths on load relative paths replaced with addresses)

@alxflw
Copy link
Contributor Author

alxflw commented Jan 19, 2022

thanks for the input y'all. I'll focus on two things:

  1. experiment with loading of files below root. I'll try the webpack config approach and see if I can get something working
  2. create a helper function and unify the importing style (aliases that are replaced with relative paths on load relative paths replaced with addresses)

ok, I tried a few things but didn't end up with a working solution for now. details:

  1. custom next.config.js: this is only allowing me to load files inside the nextjs root. we seem to be blocked by the nextjs issue referenced earlier. TS files can be loaded but arbitrary files cannot
  2. adding a utility method that could be used across web and api projects: docker compose mounts the services (web, api, db, etc) on the respective paths, so a shared utility would require changes to that setup. I feel like this would add more complexity and won't provide a lot of benefits. we'd avoid some redundant code but it'll be hard to understand why and how it is working. devs might be more confused by that

thoughts?

@alxflw
Copy link
Contributor Author

alxflw commented Jan 25, 2022

I'd love to get this to the finish line for initial release @psiemens @10thfloor. I'll resolve the conflicts but please confirm that we exhausted our options to simplify (prior comment)

Copy link
Contributor

@psiemens psiemens left a comment

Choose a reason for hiding this comment

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

Looks good overall! Just a few cleanup comments.

I say let's stick with this for now and then separately investigate better tooling to handle Cadence imports.

@alxflw alxflw requested a review from psiemens January 26, 2022 23:16
Copy link
Contributor

@psiemens psiemens left a comment

Choose a reason for hiding this comment

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

LGTM! 🚢

Congrats on first PR! 🎉

@alxflw
Copy link
Contributor Author

alxflw commented Jan 27, 2022

@10thfloor could you update your review so I could merge?

@10thfloor 10thfloor merged commit 67bb5ac into onflow:master Feb 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Import transactions/scripts into JS rather than inlining them

3 participants