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

fix: generate types for create fuels users when extracted #3175

Merged
merged 38 commits into from
Sep 26, 2024

Conversation

Dhaiwat10
Copy link
Member

@Dhaiwat10 Dhaiwat10 commented Sep 16, 2024

Closes TS-651

Summary

Currently, the artifacts generated by typegen are ignored by git. But in order for someone to deploy their dapp's frontend to the cloud, they will need these artifacts to be included in their git repo - if they follow the instructions listed on our Deploying to Testnet guide.

There is no other way to deploy the frontend since we deprecated the built-in binaries for users. This is why we need to remove these artifacts from the gitignore of the template, and include them in the source.

This PR generates these types for users when the template is extracted, and removes these files from gitignore, hence 'fixes' our deploying to testnet guide.

Checklist

  • All changes are covered by tests (or not applicable)
  • All changes are documented (or not applicable)
  • I reviewed the entire PR myself (preferably, on GH UI)
  • I described all Breaking Changes (or there's none)

Copy link

vercel bot commented Sep 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
create-fuels-template ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2024 1:40pm
ts-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 25, 2024 1:40pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
create-fuels-counter-example ⬜️ Ignored (Inspect) Sep 25, 2024 1:40pm

Copy link

codspeed-hq bot commented Sep 16, 2024

CodSpeed Performance Report

Merging #3175 will improve performances by 41.67%

Comparing dp/create-fuels-type-artifacts (6b3e4fe) with master (b6a82bc)

Summary

⚡ 1 improvements
✅ 20 untouched benchmarks

Benchmarks breakdown

Benchmark master dp/create-fuels-type-artifacts Change
should successfully execute a contract mint 95.1 ms 67.1 ms +41.67%

arboleya
arboleya previously approved these changes Sep 16, 2024
templates/nextjs/gitignore Show resolved Hide resolved
Torres-ssf
Torres-ssf previously approved these changes Sep 16, 2024
Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

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

LGTM but I wonder we should add a pretest equivalent build step that will generate these at least every build (perhaps at the root of the templates folder) if not each time we make a change to typegen or we when we release a new fuels version, otherwise we could run the risk of these becoming outdated

@Dhaiwat10 Dhaiwat10 dismissed stale reviews from Torres-ssf and arboleya via 34bb78f September 17, 2024 11:53
@Dhaiwat10
Copy link
Member Author

Good catch @maschad but this will be slightly more tricky than our usual pretest build steps because those artifacts aren't supposed to be included in the source, unlike these artifacts. We'll probably need an additonal CI step like the one that checks forc formatting, and fails if all files aren't properly formatted. Maybe we could have a similar one that fails if the geneated types are out of date? Thoughts?

@arboleya
Copy link
Member

@Dhaiwat10 I think we should always generate and override all files while not shipping pre-generated files in our repo. For example, we can have one .gitignore for the repo and override the template one during the generation.

If I got it right, @maschad meant adding something like:

{
  "scripts": {
    "pretest": "fuels build",
  },
}

@Dhaiwat10
Copy link
Member Author

@arboleya two questions:

  1. If we don't ship the generated files in our repo, then aren't we still left with the same issue that PR is trying to solve?
  2. We already have a prebuild script that generates these types but they are not added/committed to the source by the CI workflow. Does that make sense? Sorry if I'm missing something

@arboleya
Copy link
Member

@Dhaiwat10

  1. Isn't the issue happening when our users try to deploy? They must build the project to generate the files in advance before deploying, correct? Or does this apply to us as well?
  2. Yes, if the above is correct, I'm saying that we, fuels-ts, should continue ignoring these files. Then, for the app we generate for users, we can override the .gitignore (we use in the fuels-ts repo) file with another one that doesn't ignore the generated files.

@maschad
Copy link
Member

maschad commented Sep 17, 2024

We'll probably need an additonal CI step like the one that checks forc formatting, and fails if all files aren't properly formatted. Maybe we could have a similar one that fails if the geneated types are out of date? Thoughts?

@Dhaiwat10 essentially yes, what I am suggesting is a workflow that will:

  1. Generate the types for vite/next templates
  2. check the diff between the currently committed ones and the generated ones.
  3. If there is a diff, fail the job. Otherwise pass it.

@Dhaiwat10
Copy link
Member Author

types-pr-screen.mp4

@petertonysmith94 this video should demonstrate the purpose of this PR. As you can see, the sway-api folder is already populated with the types and the types have been removed from the gitignore, enabling users to deploy dapps to the cloud even if the cloud environment doesnt have forc and fuel-core available at build-time.

Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
77.18%(+0.01%) 71.8%(-0.01%) 75.77%(+0%) 77.29%(+0.01%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/create-fuels/src/cli.ts 87.95%
(+0.78%)
47.61%
(+0.25%)
100%
(+0%)
87.95%
(+0.78%)

@Torres-ssf Torres-ssf enabled auto-merge (squash) September 25, 2024 18:30
@Torres-ssf Torres-ssf disabled auto-merge September 25, 2024 18:31
@Dhaiwat10 Dhaiwat10 merged commit e7c2c30 into master Sep 26, 2024
25 checks passed
@Dhaiwat10 Dhaiwat10 deleted the dp/create-fuels-type-artifacts branch September 26, 2024 08:30
@petertonysmith94
Copy link
Contributor

types-pr-screen.mp4
@petertonysmith94 this video should demonstrate the purpose of this PR. As you can see, the sway-api folder is already populated with the types and the types have been removed from the gitignore, enabling users to deploy dapps to the cloud even if the cloud environment doesnt have forc and fuel-core available at build-time.

Beautiful video @Dhaiwat10, thanks for that. What screen grabber do you use to record this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants