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

Remove MobX-State-Tree flag for non-demo projects #2647

Open
wants to merge 27 commits into
base: v10
Choose a base branch
from

Conversation

Jpoliachik
Copy link
Contributor

@Jpoliachik Jpoliachik commented Feb 21, 2024

This PR adds support for removing MobX-State-Tree code from a non-demo project.
Retargeted to v10 branch.

Changes:

  • // @mst inline comments, similar to how demo code removal works
  • Refactored demo.ts removal logic into more generic markup.ts to be shared with @mst comment types too, and make it easy to add additional other types in the future too.
  • Added new replace-next-line markup command type, to help swap out lines of code easily without hot patching in scripts (keeps code replacements inline next to other code)

TODOs:

  • README update

(I'll also add some additional screenshots & videos for testing proof soon)

src/commands/new.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@frankcalise frankcalise left a comment

Choose a reason for hiding this comment

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

First pass, it's taking shape nice work!

src/commands/new.ts Outdated Show resolved Hide resolved
src/commands/remove-mst.ts Outdated Show resolved Hide resolved
// TODO: full dryRun support

p()
p(`Removing MobX-State-Tree code from '${TARGET_DIR}'${dryRun ? " (dry run)" : ""}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

What about a try/catch here and if running into any issues, let's warn the user the automated removal could not be completed and refer them to the recipe to do so manually?

src/commands/new.ts Outdated Show resolved Hide resolved
@frankcalise
Copy link
Contributor

@Jpoliachik can you target this against v10 as the base branch?

* feat(boilerplate): mmkv in over async storage

* fix(cli): remove async storage new arch specifics

* test(boilerplate): mmkv clean up

* fix(boilerplate): storage.load with just a string

* test(boilerplate): storage test types
@frankcalise frankcalise added this to the Ignite v10 milestone Apr 2, 2024
@Jpoliachik
Copy link
Contributor Author

@frankcalise sure thing, I'll work on this early next week 👍

@Jpoliachik Jpoliachik marked this pull request as ready for review May 9, 2024 15:30
@Jpoliachik Jpoliachik changed the title DRAFT: Remove MobX-State-Tree flag for non-demo projects Remove MobX-State-Tree flag for non-demo projects May 9, 2024
@Jpoliachik Jpoliachik changed the base branch from master to v10 May 9, 2024 15:35
@@ -63,6 +63,7 @@ export const useInitialRootStore = (callback?: () => void | Promise<void>) => {

// reactotron integration with the MST root store (DEV only)
if (__DEV__) {
// @ts-ignore
console.tron.trackMstNode(rootStore)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why this wasn't needed before, but I was getting typescript errors without.

result = sanitize(result)
} else {
result = updateFile(result, markupPrefix)
result = sanitize(result)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if this extra sanitize() was happening before - what behavior do we want here, I assumed we'd always want to get rid of all the markup regardless

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.

None yet

3 participants