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

refactor(cli): refactor to esm #417

Merged
merged 13 commits into from
Feb 22, 2023
Merged

refactor(cli): refactor to esm #417

merged 13 commits into from
Feb 22, 2023

Conversation

dk1a
Copy link
Contributor

@dk1a dk1a commented Feb 19, 2023

Replaces #397

From my testing this is the best solution for table autogeneration, and should be neutral for current use-cases.

Latest main should be merged into v2 before this PR (network's createFaucetService is duplicated in both)

There're no significant logic changes here, but I remove some seemingly deprecated commands to do less refactoring (deploy, diamond-abi, sync-art).
Added reuseComponents to deploy-contracts since it was only in deploy.

I've started going from no bundler+ts-node-esm+moduleResolution nodenext (which doesn't work well yet) to eventually tsup with some inlining+node+moduleResolution node. The changes that make the 1st more extreme version work could be removed, but they are a strict improvement and could simplify things in the future.
Notably:

  • .js suffixes for files
  • services changes
  • tsc --moduleResolution nodenext
  • execLog refactor was just me testing stuff, but it should be a strict improvement worth keeping

@holic create-mud was working without @latticexyz/utils seemingly because it was cli's dependency, even though cli didn't (and couldn't) use it. And since it's std-client's peerDep it's best to add it anyways.

@authcall importSuffix=.js helps with future compatibility with es module resolution. (It'd be even better if ts-proto and protobufjs would replace long with bigint entirely protobufjs/protobuf.js#1557).

@@ -1,47 +0,0 @@
import { Arguments, CommandBuilder } from "yargs";
Copy link
Member

Choose a reason for hiding this comment

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

@Kooshaba this is not used in sky strife anymore, is it?

Copy link
Member

Choose a reason for hiding this comment

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

chatted on discord, seems like sky strife is actually still using this command, so we shouldn't remove it for now

@alvrs alvrs changed the title feat: refactor cli to esm refactor(cli): refactor to esm Feb 21, 2023
Copy link
Member

@alvrs alvrs 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 to me! Tested a couple commands locally and everything still works as expected. Good to go once main was merged into v2 to avoid a conflict with the changes in createFaucetService

Copy link
Member

@alvrs alvrs left a comment

Choose a reason for hiding this comment

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

unfortunately we actually do need sync-art, see #417 (comment)

Copy link
Member

@alvrs alvrs left a comment

Choose a reason for hiding this comment

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

(or maybe sync-art could be a standalone script outside the mud cli)

@holic
Copy link
Member

holic commented Feb 22, 2023

(or maybe sync-art could be a standalone script outside the mud cli)

Yeah this would be my preference, since sync-art feels very Phaser specific (if not Sky Strife specific)

@dk1a
Copy link
Contributor Author

dk1a commented Feb 22, 2023

@alvrs rebased; test some commands again just to make sure it still works.
There were quite a lot of conflicts, some rather indirect. I think to avoid dealing with those often it'd be best to avoid cli changes in main. Or merge v2 into main, if we're ok with a few breaking cli changes and a bunch of unfinished packages there.

(or maybe sync-art could be a standalone script outside the mud cli)

Yeah this would be my preference, since sync-art feels very Phaser specific (if not Sky Strife specific)

It makes sense to put that script into sky strife repo. It's small, self-contained and very project-specific. Leaving sync-art removed for now, unless you disagree

@alvrs
Copy link
Member

alvrs commented Feb 22, 2023

There were quite a lot of conflicts, some rather indirect. I think to avoid dealing with those often it'd be best to avoid cli changes in main. Or merge v2 into main, if we're ok with a few breaking cli changes and a bunch of unfinished packages there.

Yes, I think that makes sense, I'm all for merging v2 into main as soon as possible.

It makes sense to put that script into sky strife repo. It's small, self-contained and very project-specific. Leaving sync-art removed for now, unless you disagree

Agreed

@alvrs
Copy link
Member

alvrs commented Feb 22, 2023

The gas-report cli is failing because the gasreport script in store still refers to cli/dist/index.js instead of cli/dist/mud.js - we could probably change that now to use the released @latticexyz/cli gas-report command

@dk1a
Copy link
Contributor Author

dk1a commented Feb 22, 2023

The gas-report cli is failing because the gasreport script in store still refers to cli/dist/index.js instead of cli/dist/mud.js - we could probably change that now to use the released @latticexyz/cli gas-report command

Right, missed that one

@alvrs alvrs merged commit 8998ae4 into v2 Feb 22, 2023
@dk1a dk1a deleted the dk1a/cli-esm branch May 9, 2023 16:23
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.

3 participants