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): split commands into separate directories #407

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

agilgur5
Copy link
Collaborator

@agilgur5 agilgur5 commented Dec 31, 2019

  • should be easier to read & understand the codebase as there's a
    better separation of concerns now

    • don't need to look at / concern oneself with build code when editing
      lint code, for instance
  • should also make future refactors much easier, as no need to parse
    or change a big file with all the commands and logic

  • also split up the command logic from the action logic as two separate
    functions for each command

This is built on top of #406 , so please merge that one in first (can rebase this after) if this is to be merged. EDIT: rebased now that #406 has been merged.
This has been something I've been looking to do since #367 , as this makes it much easier to wade through the codebase imo. src/index.ts was fairly unwieldy beforehand. src/build/index.ts and src/create/index.ts are getting there, but I think this is much better and this new structure makes it easier to refactor them in the future.

There's still 1 test failing here, the build-invalid one (somehow it succeeds), so need to investigate and fix that still, but otherwise this PR is complete

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jan 1, 2020

weird, the build-invalid test fails locally on my machine, but passes here...

here's the failure I'm getting:

 FAIL  test/tests/tsdx-build.test.js (19.485s)
  ● tsdx build › should fail gracefully with exit code 1 when build failed

    expect(received).toBe(expected) // Object.is equality

    Expected: 1
    Received: 0

      82 |     util.setupStageWithFixture(stageName, 'build-invalid');
      83 |     const code = shell.exec('node ../dist/index.js build').code;
    > 84 |     expect(code).toBe(1);
         |                  ^
      85 |   });
      86 | 
      87 |   it('should only transpile and not type check', () => {

      at Object.it (tests/tsdx-build.test.js:84:18)

- should be easier to read & understand the codebase as there's a
  better separation of concerns now
  - don't need to look at / concern oneself with build code when editing
    lint code, for instance
- should also make future refactors much easier, as no need to parse
  or change a big file with all the commands and logic

- also split up the command logic from the action logic as two separate
  functions for each command
@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jan 4, 2020

Ok, the occasional missing .d.ts bug in Ubuntu CI errors (c.f. #403 ) was repro'd here on macOS, which confirms the error I was getting locally on macOS at #367 (comment) . Not sure if the missing .d.ts errors also occur on Windows, don't think they've been repro'd there yet (though Windows also runs the longest and so gets cancelled most often), so maybe this error is exclusive to Unix systems 😕 idk tho

@jaredpalmer
Copy link
Owner

Just skimming, but I’m not a fan of refactoring for the sake of it. I’d sooner move everything into one gigantic file than split it into folders.

@agilgur5
Copy link
Collaborator Author

agilgur5 commented Jan 5, 2020

I've made a bunch of changes to the build code and I personally found it very difficult to parse the one gigantic file, especially as it has functions all over the place, and functions in functions in functions. I did not refactor this "for the sake of it", it also was not a trivial endeavor to do.

Frequently when changing build code I had to scroll through or accidentally get confused by code for test, lint, or create as those commands are interspersed between all the build code. And then I'd have to check if certain functions were build-only or if they were used by others when changing them (they're all only used by a single command). There's other challenges and tediousness too, like gigantic import block with all commands' imports, many of which are only used by one command.

There's a lot of mental overhead to keeping it in one gigantic file, it's much harder to keep all the details in your head.

The only shared code is that of utils and constants, the commands have almost entirely independent code (they serve different functions after all), meaning that each of their code, functions, imports are almost entirely unrelated to the other commands. Notably, I have rarely ever touched the create/test/lint code in all my changes. BUT, I still had to read through, get confused by, work around, keep in my head, etc that completely unrelated code during all the build changes. This makes it harder to change and less approachable to new contributors IMO. And the file is only going to grow in size & complexity over time.

The code that's already been extracted to separate files for create/test/lint I've basically never read outside of this change. That makes it a good abstraction IMO. Putting the commands in separate files is a logical extension to improve that abstraction. Directories goes a step further in that you don't even need to see the files of unrelated commands if you're not changing them. No need to worry about them or add mental overhead.

Would you be okay with separate files for each command at least? I think that's the minimum to make the gigantic file easier to approach. The tests for each command are also already separate files.

@agilgur5
Copy link
Collaborator Author

#31 goes over how create shares little code with the other commands too

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.

2 participants