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

[Request] Organize project and tests in multiple files for V2 #12

Open
joshuafcole opened this issue Jul 11, 2014 · 4 comments
Open

[Request] Organize project and tests in multiple files for V2 #12

joshuafcole opened this issue Jul 11, 2014 · 4 comments

Comments

@joshuafcole
Copy link
Contributor

Continuing on the discussion from my PR, I think it would be valuable to split version 2 up into various smaller files that can be minified together for usage on the web, or used directly in Node.

There are a handful of benefits to this --

1. Easier for newcomers to grok

Understanding and contributing to foreign codebases can be a daunting task. One way to make that simpler is to divide the code into obvious chunks that can be consumed one at a time. While whitespace chunking and comments are generally sufficient for people who are familiar with the codebase, they require at least one (and possibly several) full read throughs of the codebase before a new programmer can reliably use them.

To give an example of this, I recently contributed to V1 to add support for the LIKE and IN operators. The actual changes were very simple (one line in CondLexer's readWord() method), but actually finding that out was a fairly intensive process. If there had been a separate file for the CondLexer and CondParser, I would have quickly taken a look at one or the other, and if I had gone to CondParser, it would have lead me to CondLexer very quickly.

Since you already have a build system in place via grunt, the actual cost of adding the minification step for the web is basically nil, you even get the benefit of more useful stack traces in node (assuming you use the non-catted files).

2. Easier to write comprehensive tests for

Like the main library, the test suite is currently also in a single file. If it were split into multiple files (preferably multiple files of many separate tests) along the same boundaries as the source files they test, I could have just that file of tests run on every change I make, alerting me immediately to mistakes rather than waiting until I break out of flow to manually run the tests and check. Running the entire test suite instead of just the relevant ones is much more time consuming and tends to only add noise to the signal.

3. Better editor/tooling support

Smaller files let tools such as JSHint/ESLint and JS-Beautifier run faster. Many editors (such as my own) offer integration with these tools that help ensure high quality code. However, on very large files, they can cause intrusive lag and need to be disabled.

@dsferruzza
Copy link
Owner

Hi!

Thanks for the detail.
Here is my reaction:

  1. I agree that it would be easier for new contributors to begin with the project if it is well split into several files.
  2. Splitting tests can be a good idea for the same reasons as 1. Actually, it would be easier to do, because tests are independent from each other (though they depend on some helpers). But reducing testing duration is not (for now) really necessary:
    • from command line, phantomjs is slower to launch than the tests to run
    • from the browser, you can run a particular test if you want to (by clicking on Rerun).
  3. I can understand the pain.

So I'm OK to do this.
Here are some problems that need to be solved in order to do a good work:

  • find a good separation to maximize maintainability: some parts of the code are coupled, so this might not be straightforward
  • redesign the build system to make the project work in node and in browser with no friction at all for the user (maybe use Browserify?). I want to support these ways of installation:
    • node: npm
    • browser: bower, manual download from GitHub

What do you think?

@joshuafcole
Copy link
Contributor Author

Yeah, test speed right now is pretty good. Hopefully it never applies here, but at work I've dealt with some older projects that have test suites that take up to 10-15 seconds to run.

In response to your points:

  1. Yep, figuring out the right divides is always the toughest part of the job. I've found that it helps to start off just by thinking of things that are either logically independent or have a strict one way dependency (e.g. a parser on a lexer) and treating those as blocks to rearrange. Some blocks are very tiny and can stay together, but others are large (or just complex) enough that it makes sense to make them separate. Since there's already a pretty feature-packed V1, there's time to play with things and reorganize if you don't like where it's going.
  2. I agree, that's a good idea, and I can't recommend Browserify highly enough. I started experimenting with it on my own time last year and loved it so much that I've since gotten my entire company to rebuild our stack from RequireJS to Browserify. It's almost entirely painless and works almost exactly like Node in the browser. At work we switched from Grunt to Gulp at the same time, so I don't know which contrib plugin exactly to use with Grunt, but I imagine it can't be very different or complicated. If you need help here I'd be glad to do so.

@dsferruzza
Copy link
Owner

I switched the project to CommonJS.
The build tasks use Browserify to generate 4 bundles:

  • with and without dependencies
  • plain and minified

See: 4d7db7d...58a42c6

@dsferruzza
Copy link
Owner

I split the code in 2 files and refactored tests.
So I guess this issue can be closed.
What do you think?

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

No branches or pull requests

2 participants