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.

Sign up for GitHub

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

Jump to bottom

Add forc-bin package and upgrade to Forc v0.14.4 #287

Merged
merged 8 commits into from
Jun 1, 2022
Merged

Add forc-bin package and upgrade to Forc v0.14.4 #287

merged 8 commits into from
Jun 1, 2022

Conversation

AlicanC
Copy link
Contributor

@AlicanC AlicanC commented May 19, 2022 •
edited
Loading

This PR is some progress on #282.

feat: add forc-bin package

Adds a new package called forc-bin. Packages that need Forc can add this as a dev-depencency and can use it with pnpm forc. This package can eventually be moved to Sway repo (https://github.com/FuelLabs/pm/issues/19).

You can also run pnpm forc:update any time, which will:

  • update forc-bin with the latest version on GitHub (if necessary)
  • remove all Forc.locks
  • run pnpm build to build all Forc projects

feat: upgrade to Forc v0.13.0

Makes the necessary changes for Forc upgrade.

chore: clean build artifacts

Just a separate commit for visibility. We don't need to commit these anymore.

@AlicanC AlicanC force-pushed the jc/improve-forc-usage branch 2 times, most recently from 24f3276 to 51de976 Compare May 20, 2022 16:22
@AlicanC AlicanC changed the title Improve Forc usage Add forc-bin package and upgrade to Forc v0.13.0 May 20, 2022
@AlicanC AlicanC added the feat Issue is a feature label May 20, 2022
@AlicanC AlicanC marked this pull request as ready for review May 20, 2022 16:36
@AlicanC
Copy link
Contributor Author

AlicanC commented May 20, 2022

Noticed some problems after I've asked for reviews and pushed fixes.

adlerjohn
adlerjohn reviewed May 20, 2022
View reviewed changes
Copy link
Contributor

@adlerjohn adlerjohn 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

QuinnLee
QuinnLee previously approved these changes May 20, 2022
View reviewed changes
Copy link
Contributor

@QuinnLee QuinnLee left a comment

Choose a reason for hiding this comment

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

:shipit:

luizstacio
luizstacio previously requested changes May 20, 2022
View reviewed changes
packages/forc-bin/src/install.ts
(async () => {
const pkgPlatform = getPkgPlatform();
const pkgVersion = await getCurrentVersion();
const pkgName = `forc-binaries-${pkgPlatform}.tar.gz`;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move all this URLs and repo names to a config file on the root

scripts/forc-update.ts Outdated Show resolved Hide resolved
scripts/forc-update.ts Show resolved Hide resolved
camsjams
camsjams reviewed May 20, 2022
View reviewed changes
Copy link
Contributor

@camsjams camsjams left a comment

Choose a reason for hiding this comment

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

LGTM, small comment

@AlicanC AlicanC changed the title Add forc-bin package and upgrade to Forc v0.13.0 Add forc-bin package and upgrade to Forc v0.13.2 May 26, 2022
camsjams
camsjams previously approved these changes May 26, 2022
View reviewed changes
Copy link
Contributor

@camsjams camsjams left a comment

Choose a reason for hiding this comment

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

This looks good, some of the code from scripts/forc-update.ts is similar to that inside the forc-bin package, but I understand why its in a different location.

@AlicanC
Copy link
Contributor Author

AlicanC commented May 26, 2022

To clear it up further:

How we ideally want to use Forc is discussed on #282. This PR is just some progress on the issue and has no desire to fully resolve it.

This PR should be a strict improvement to the project without any regressions, and should also easily be upgradeable to whatever we want to have in the future.

My motivation is that even though I can keep this next to my other local scripts and just send an Update Forc to X.Y.Z PR whenever I use it, I believe this can be useful for the rest of the team (even at this stage), and also be a guide to whoever wants to fully tackle #282 (and https://github.com/FuelLabs/pm/issues/19).

To clear it even further:

  • forc-bin should be a dumb project. It's inspired from flow-bin which is very basic and has even very critical files untouched for months or even years. This isn't something I'm willing to innovate on, but just follow.
  • forc-bin should be in the Sway repo, and not export install.js, update.js or anything like that, but just index.js that exports the path to the binary and cli.js for the NPM bin. In this setup, scripts/forc-update.ts:
    • Won't be able to import anything useful from forc-bin. (This is why some obvious things weren't shared.)
    • Will run pnpm -w -D install forc-bin instead of pnpm --filter forc-bin run update.
  • scripts/forc-update.ts should also run a command that only builds Forc projects instead of the full pnpm build.

@camsjams
Copy link
Contributor

v0.14.0+ now has predicate support

@AlicanC AlicanC requested a review from camsjams June 1, 2022 08:47
@AlicanC AlicanC changed the title Add forc-bin package and upgrade to Forc v0.13.2 Add forc-bin package and upgrade to Forc v0.14.4 Jun 1, 2022
@AlicanC
Copy link
Contributor Author

AlicanC commented Jun 1, 2022

v0.14.0+ now has predicate support

Updated to 0.14.4 👍

adlerjohn
adlerjohn approved these changes Jun 1, 2022
View reviewed changes
Copy link
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

Looks ok, but will defer to the ts experts. Can you file an issue to follow-up with readme documentation on how to bump the version?

@AlicanC AlicanC merged commit 4629817 into master Jun 1, 2022
@AlicanC AlicanC deleted the jc/improve-forc-usage branch June 1, 2022 16:45
@AlicanC AlicanC mentioned this pull request Jun 1, 2022
Merged
@luizstacio luizstacio linked an issue Jun 15, 2022 that may be closed by this pull request
Remove stale binaries from repository #213
Closed
@AlicanC AlicanC mentioned this pull request Jun 16, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuinnLee QuinnLee QuinnLee approved these changes

@adlerjohn adlerjohn adlerjohn approved these changes

@luizstacio luizstacio luizstacio approved these changes

@pedronauck pedronauck Awaiting requested review from pedronauck

@camsjams camsjams Awaiting requested review from camsjams

Assignees

@AlicanC AlicanC

Labels
feat Issue is a feature
Projects
Fuel Network
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove stale binaries from repository
5 participants
@AlicanC @camsjams @QuinnLee @adlerjohn @luizstacio