Skip to content

feat(create-mud): add create-mud package#336

Merged
alvrs merged 10 commits into
mainfrom
holic/create-mud
Feb 10, 2023
Merged

feat(create-mud): add create-mud package#336
alvrs merged 10 commits into
mainfrom
holic/create-mud

Conversation

@frolic

@frolic frolic commented Jan 10, 2023

Copy link
Copy Markdown
Member

Generated with create-create-app (using my fork to fix some issues).

Example using npm create mud [project name]:
image
(Just uses a template option for now, but we can add more things as needed)

Main benefit for me is that the template code is now colocated with the rest of MUD, so we can update references/dependencies together in single commits/PRs.

I'd like to eventually set up some CI that can create + build projects from this new CLI (to make sure everything is happy) but will save that for a follow up.

One thing we may run into is that publishing to npm may ignore/not include the .gitignore files as part of the template output, so we may need to try a few things:

@dk1a

dk1a commented Jan 10, 2023

Copy link
Copy Markdown
Contributor

I think test in foundry.toml (which is not set and test by default) and the de facto test dir src/test should be syncd. (This is particularly relevant for #309, in its current state at least)

@frolic frolic marked this pull request as ready for review February 9, 2023 00:26
@frolic frolic requested a review from alvrs as a code owner February 9, 2023 00:26
@frolic

frolic commented Feb 9, 2023

Copy link
Copy Markdown
Member Author

This is ready to go! Didn't need much to bring it up to speed.

I made sure that the resulting npm package will have the necessary .gitignore in templates by adding an .npmignore to templates dir. I tested this with npm pack which outputs the files it bundles into the tarball, and could see the template .gitignore files appear after this change.

I also updated MUD dependencies in each template and made sure each works by first building create-mud with yarn build, then running dist/cli.js test-package to create a new package for minimal+react templates, and made sure they both boot up and run properly (can increment counter and see the UI update).

@alvrs alvrs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good! Just had a few minor questions. For a follow up - how hard would it be to make the templates "modular" with the create-create-app setup? Eg. the contracts setup is probably the same for different client templates, so we could just define it once and then copy it into the generated project. Or maybe in the future we could have different contract templates that can be mixed-and-matched with different client templates.

Comment thread packages/create-mud/LICENSE Outdated
@@ -0,0 +1,19 @@
Copyright (c) 2023 Lattice <mud@lattice.xyz>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need a separate licence file in the package? If so, should we align it with the global one (MIT): https://github.com/latticexyz/mud/blob/main/LICENSE ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oops, I think this one was auto created by create-create-app

Comment thread packages/create-mud/package.json Outdated
"version": "0.0.0",
"author": "Lattice <mud@lattice.xyz>",
"scripts": {
"postinstall": "cd node_modules/create-create-app && npm run build",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what does this do?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

since I'm using my own (unpublished) fork, this makes sure the create-create-app is built before using

Comment thread packages/create-mud/package.json Outdated
"scripts": {
"postinstall": "cd node_modules/create-create-app && npm run build",
"build": "tsup src/cli.ts --minify",
"clean": "shx rm -rf lib",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

uhh I like this, maybe we should use shx in all our packages. I don't see shx in the dependencies though, where does it come from?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this was also included by create-create-app, looks like a transitive dependency (but prob should be a real dev dep): https://github.com/uetchy/create-create-app/blob/master/package.json#L53

@alvrs

alvrs commented Feb 9, 2023

Copy link
Copy Markdown
Member

Oh also to @dk1a comment, should we move test out of the src folder for the two templates? (Could also do it in a follow up, since the focus of this PR is not to update the templates themselves but just update the project generation flow)

alvrs
alvrs previously approved these changes Feb 9, 2023

@alvrs alvrs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

preliminary approving this!

@alvrs

alvrs commented Feb 9, 2023

Copy link
Copy Markdown
Member

Just thought of two more things: we need to add create-mud to the root's package.json under packages, and we should add a test script to create-mud's package.json (even if it's just a stub like "echo 'no tests'" for now) to make the global test script work. (It just didn't run the test script so far for the create-mud package because it wasn't in the root's packages list)

@frolic

frolic commented Feb 10, 2023

Copy link
Copy Markdown
Member Author

Oh also to @dk1a comment, should we move test out of the src folder for the two templates? (Could also do it in a follow up, since the focus of this PR is not to update the templates themselves but just update the project generation flow)

Moving src/test to test gives me this error with mud test:

[Error: ENOENT: no such file or directory, open 'src/test/LibDeploy.sol'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: 'src/test/LibDeploy.sol'
}

Looks like because our default in mud test is src/test?
image
Should we change this? or manually configure mud test to use test as the dir? or include a diff test dir in foundry.yaml?

@dk1a

dk1a commented Feb 10, 2023

Copy link
Copy Markdown
Contributor

Looks like because our default in mud test is src/test?

yep, mud cli has a bunch of hardocded paths (src/test alone is used in test and call-system it seems)

Should we change this? or manually configure mud test to use test as the dir? or include a diff test dir in foundry.yaml?

I address some of that in #309. That may be best left for v2. For now I had thought you'd just add test = "src/test" to foundry.toml to make the current situation explicit

@frolic

frolic commented Feb 10, 2023

Copy link
Copy Markdown
Member Author

I address some of that in #309. That may be best left for v2. For now I had thought you'd just add test = "src/test" to foundry.toml to make the current situation explicit

Done!

I think in the future, we should probably use a top-level test dir rather than inside src so that it'll be easier to build Solidity artifacts (e.g. ABIs) that don't also include test artifacts.

Edit: opened an issue for it here: #389

@frolic frolic requested a review from alvrs February 10, 2023 20:47

@alvrs alvrs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

good to go! 🚀

@alvrs alvrs enabled auto-merge February 10, 2023 20:50
@alvrs alvrs added this pull request to the merge queue Feb 10, 2023
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Feb 10, 2023
@frolic frolic added this pull request to the merge queue Feb 10, 2023
@frolic

frolic commented Feb 10, 2023

Copy link
Copy Markdown
Member Author

merge queue stalled, I think #391 should fix it

@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Feb 10, 2023
@alvrs alvrs added this pull request to the merge queue Feb 10, 2023
Merged via the queue into main with commit e85c124 Feb 10, 2023
@alvrs alvrs deleted the holic/create-mud branch May 18, 2023 12:52
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