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

[chore]: replace esbuild with swc in flat-web #984

Closed
BlackHole1 opened this issue Sep 29, 2021 · 5 comments
Closed

[chore]: replace esbuild with swc in flat-web #984

BlackHole1 opened this issue Sep 29, 2021 · 5 comments
Assignees
Labels
enhancement New feature or request stale Stale issues or PR

Comments

@BlackHole1
Copy link
Collaborator

BlackHole1 commented Sep 29, 2021

Our mobx class is starting to look more and more complex. Using reflect-metadata can be a good solution to this problem.

currently flat-web uses vite, and uses vite's built-in esbuild build tool.

But currently esbuild does not support reflect-metadata, which is the main reason why we want to migrate to swc.

Since the implementation logic of esbuild and swc is completely inconsistent, it will be difficult to support esbuild in the future, see: evanw/esbuild#257 (comment)

Related Links: vitejs/vite#788

@BlackHole1 BlackHole1 added enhancement New feature or request good first issue Good for newcomers labels Sep 29, 2021
@BlackHole1 BlackHole1 changed the title [chore]: replace esbuil with swc in flat-web [chore]: replace esbuild with swc in flat-web Sep 29, 2021
@BlackHole1
Copy link
Collaborator Author

BlackHole1 commented Sep 29, 2021

after an internal team discussion, we felt that this feature should not go first.

our core problem is that the existing mobx Store is difficult to maintain.

the reflect-metadata is just a solution to try to solve the problem, but it should not be finalized directly, but should be researched first to see if there are other solutions to solve the problem.

currently mobx v6 does not recommend using decorators anymore, I think we should use the official recommendation (makeObservable / makeAutoObservable) (Although we are using these right now...)


Let me elaborate on the current pain points of our mobx Store:

1

class TestStroe {
    constructor() {
        makeObservable(this, {
            value: observable,
            double: computed,
            increment: action,
            fetch: flow,
        });

        // imagine that there are more than 400 lines of code

        // when I get here, how do I know if it's `computed` or `ref`?
        // the frustrating thing is that you have absolutely no idea! 
        // you need to scroll your mouse until you see `makeObservable` and you can confirm
        double() {
            // code...
        }
    }
}

2

our Store is too big! So big that you can open any Store file and feel a deep sense of despair...

they should have had the opportunity to split into more granular Stores...

@BlackHole1 BlackHole1 removed the good first issue Good for newcomers label Sep 29, 2021
@crimx
Copy link
Member

crimx commented Sep 29, 2021

We may address the issues by enforcing naming convention and further splitting stores.

@BlackHole1
Copy link
Collaborator Author

can mobx-state-tree solve our problem?

cc @crimx @l1shen

@crimx
Copy link
Member

crimx commented Sep 29, 2021

I've investigated the mobx-state-tree before adopting mobx to Flat. I think it won't solve our issues.

  1. We don't need snapshots and runtime type checking which are the main selling functionalities of mobx-state-tree.
  2. It adds another layer of concept complexity and runtime overhead which Flat doesn't need.
  3. It offers poor supports for async await. You need to write generators for async actions which looks quite nasty.

@stale
Copy link

stale bot commented Nov 28, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale issues or PR label Nov 28, 2021
@stale stale bot closed this as completed Dec 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request stale Stale issues or PR
Projects
None yet
Development

No branches or pull requests

4 participants