Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Split the codebase #378

Closed
jzaefferer opened this issue Dec 18, 2012 · 12 comments
Closed

Split the codebase #378

jzaefferer opened this issue Dec 18, 2012 · 12 comments
Labels
Category: Release Type: Enhancement New idea or feature request.
Milestone

Comments

@jzaefferer
Copy link
Member

Up for discussion: Currently the QUnit code base is just a single js file (with companion css). That makes it possible to include QUnit as a submodule, from tag or latest, no need to build anything. It also makes maintenance kind of a pain, as we're now dealing with 2k+ LoC in a single file. Finding things isn't easy. A good bunch of code lends itself to be moved to separate modules, like QUnit.eqiv or QUnit.jsDump. That would make cleanup of the remaining pieces a lot easier.

When working on QUnit, we could use grunt watch to trigger a build for every change, similar how development on jQuery Core works.

As for usage of the QUnit "binary":

  • jQuery Core uses QUnit as a submodule: That could be replaced by npm install qunitjs and referencing node_modules/qunit/ directly or copying the files to whereever they're supposed to live. npm modules can contain "binaries", so whatever the build outputs can be published through npm, without commiting it to Git.
  • jQuery UI and jQuery Mobile include QUnit as copies in its externals folder. That is updated manually by copying over the files, so that shouldn't be a problem at all.

What else is there that we need to take into account?

@jzaefferer
Copy link
Member Author

This would also make #364 much less painful to resolve.

@rodneyrehm
Copy link
Contributor

+1 for splitting the codebase. I've been trying to get a grip on QUnit's source for some time now. It's a pain. A single file should only concern itself with a single topic - and there are a bunch of them:

  • core (initialization, state, event bus, …)
  • module, test, … ("organizational infrastructure")
  • assertions ()
  • output visualization ("rendering the DOM", not required if you're running JS-only, btw.)
  • diff algorithm
  • diff visualization ("rendering", not required for JS-only)
  • exception handling (btw. is TraceKit something?)
  • probably more…

that would also allow throwing the addons into the central source directory.

I'm fond of RequireJS and UMD. Since we're dealing with a test suite (that is not served to millions of people) I don't see the point in building a single optimized file.

@JamesMGreene
Copy link
Member

I think splitting this up is a great idea. Like @rodneyrehm, I also love RequireJS and UMD. However, I think it is important to build a single optimized "qunit.js" file for all those consumers who have not yet adopted RequireJS/AMD as their standard (which is also why we would use a form of UMD instead of AMD).

As far as UMD wrappers go, I've been the most impressed by what @jdalton did with Lo-Dash and I've been modeling my own UMD wrappers after that.

@dmethvin
Copy link

Having just looked at adding a feature to QUnit I am now on the side of splitting this up and having a build process. It would be nice to have the markup in a template that got compiled to JavaScript so there wouldn't be so much string slinging throughout.

@JamesMGreene
Copy link
Member

I'd love to see the HTML pretty much sliced out all together (as feasible) so that QUnit could support multiple reporters types (like Mocha does) with minimal DOM overhead.

@Krinkle
Copy link
Member

Krinkle commented Mar 7, 2013

Reminder: As part of splitting it up in a more modular code base, also rename the qunit/ directory to src/.

@jdalton
Copy link

jdalton commented Mar 7, 2013

fwiw I get QUnit working in Rhino, Rhino --require, Ringo, Narwhal, Node, & browsers using:

  /** Use a single "load" function */
  var load = typeof require == 'function' ? require : window.load;

  /** The unit testing framework */
  var QUnit = (function() {
    var noop = Function.prototype;
    return  window.QUnit || (
      window.addEventListener || (window.addEventListener = noop),
      window.setTimeout || (window.setTimeout = noop),
      window.QUnit = load('../vendor/qunit/qunit/qunit.js') || window.QUnit,
      (load('../vendor/qunit-clib/qunit-clib.js') || { 'runInContext': noop }).runInContext(window),
      addEventListener === noop && delete window.addEventListener,
      window.QUnit
    );
  }());

I shim timers for Rhino, setTimeout et all, via qunit-clib.
The addEventListener hack-around is smth I had to add to get it to work in recent versions.
The setTimeout of noop is to fake out QUnit's detections so I can shim it via qunit-clib.
After QUnit is loaded I remove the addEventListener hack-around.

@jzaefferer
Copy link
Member Author

@jdalton thanks for sharing! The addEventListener shim shouldn't be necessary anymore once #401 lands.

@Krinkle
Copy link
Member

Krinkle commented Jun 12, 2013

src/
 - intro.js   // header + open closure
 - core.js
 - events.js  // issue #422
 - test.js    // Test constructor
 - assert.js  // QUnit.assert / QUnit.Assertion (issue #374)
 - diff.js    // QUnit.jsDiff
 - dump.js    // QUnit.jsDump
 - reporter/
   - html.js
 - exports.js // window + module.exports
 - outro.js   // end closure

@JamesMGreene
Copy link
Member

👍

What about a Module/Suite constructor?

@jzaefferer
Copy link
Member Author

When we do the split, we should remove the version numbers from source files and add those back as part of the build step. That way releases should become easier as well.

@jzaefferer
Copy link
Member Author

For bower publishing (see #461) we should change the release process to generate the current concatenated files including version numbers, commit that in a branch and tag it. jQuery UI does the same thing.

Krinkle added a commit that referenced this issue Oct 10, 2013
Order is mostly preserved, a few changed had to be made (otherwise
we had to create a core-before.js and core-after.js)

- Prototype split
  This used to be in the middle after Test and Assert. It is now
  right after QUnit is first defined.
  So instead of things becoming globals depending on whether
  they're before or after the prototype split, they now become
  globals depending on whether they use QUnit or
  QUnit.constructor.prototype (some parts of the code used this
  already). Removed the, now redundant, comment about the order
  in assert.js (issue #341)

- Test
  Was close to the top inside core, this is now in its own
  modudle after core.
  No side effects (constructor is still hoisted, prototype now
  assigned later, neither is used until the load event).

- Assert
  Was close to the top inside core, this is now in its own
  module after core and Test.
  Removed the `assert` property from the `extend(QUnit, { .. })`
  in core because that now too early (assert is undefined).
  Moved the assignment to assert.js

- Export
  Due to the reliance on position of the prototype split, the
  export for browser globals was somewhere in the middle and
  the one for CommonJS modules on the bottom. They are now both
  at the bottom.

Build process:

- Based on the build process of jQuery and jQuery UI.

Clean up:
- gitignore: Alphabetized.
- gitignore: 'dist/' was already in there apparently, removed
  redundant slash (matching jQuery).
- package.json: Match order of properties with jQuery.
- package.json: Removed 'contributors' property.
- grunt: Removed old build-git task, like jquery, we don't
  include the git hash. Instead we have 'pre' in the version and
  an exact timestamp.
- src: Simplified file headers to be more like jQuery and made
  them match between js and css.

Fixes #378.
Krinkle added a commit that referenced this issue Oct 10, 2013
Order is mostly preserved, a few changed had to be made (otherwise
we had to create a core-before.js and core-after.js)

- Prototype split
  This used to be in the middle after Test and Assert. It is now
  right after QUnit is first defined.
  So instead of things becoming globals depending on whether
  they're before or after the prototype split, they now become
  globals depending on whether they use QUnit or
  QUnit.constructor.prototype (some parts of the code used this
  already). Removed the, now redundant, comment about the order
  in assert.js (issue #341)

- Test
  Was close to the top inside core, this is now in its own
  modudle after core.
  No side effects (constructor is still hoisted, prototype now
  assigned later, neither is used until the load event).

- Assert
  Was close to the top inside core, this is now in its own
  module after core and Test.
  Removed the `assert` property from the `extend(QUnit, { .. })`
  in core because that now too early (assert is undefined).
  Moved the assignment to assert.js

- Export
  Due to the reliance on position of the prototype split, the
  export for browser globals was somewhere in the middle and
  the one for CommonJS modules on the bottom. They are now both
  at the bottom.

Build process:

- Based on the build process of jQuery and jQuery UI.

Clean up:
- gitignore: Alphabetized.
- gitignore: 'dist/' was already in there apparently, removed
  redundant slash (matching jQuery).
- package.json: Match order of properties with jQuery.
- package.json: Removed 'contributors' property.
- grunt: Removed old build-git task, like jquery, we don't
  include the git hash. Instead we have 'pre' in the version and
  an exact timestamp.
- src: Simplified file headers to be more like jQuery and made
  them match between js and css.

Fixes #378.
@ghost ghost assigned Krinkle Oct 10, 2013
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Category: Release Type: Enhancement New idea or feature request.
Development

No branches or pull requests

6 participants