Split core.js into smaller chunks #785

Closed
wants to merge 28 commits into
from

Conversation

4 participants
@BraulioVM
Contributor

BraulioVM commented Mar 17, 2015

This PR tries to close #782.

I make the pull request now so that anyone can see how I'm approaching the task and can tell me whether I'm doing something wrong. As it is work in progress, it may not pass the tests

@BraulioVM

This comment has been minimized.

Show comment
Hide comment
@BraulioVM

BraulioVM Mar 17, 2015

Contributor

CI not working because of this: #782 (comment)

Contributor

BraulioVM commented Mar 17, 2015

CI not working because of this: #782 (comment)

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Mar 17, 2015

Its "logging", two G.

Its "logging", two G.

src/core/utilities.js
+ * @param {String|Error} error
+ * @return {String} error message
+ */
+function errorString ( error ) {

This comment has been minimized.

@jzaefferer

jzaefferer Mar 17, 2015

Member

I wonder where this function is actually getting used - maybe move it to stacktrace.js?

@jzaefferer

jzaefferer Mar 17, 2015

Member

I wonder where this function is actually getting used - maybe move it to stacktrace.js?

This comment has been minimized.

@BraulioVM

BraulioVM Mar 17, 2015

Contributor

It's just being used in assert.js, would it make sense to put it inside that file?

@BraulioVM

BraulioVM Mar 17, 2015

Contributor

It's just being used in assert.js, would it make sense to put it inside that file?

@BraulioVM

This comment has been minimized.

Show comment
Hide comment
@BraulioVM

BraulioVM Mar 17, 2015

Contributor

I'm not sure why the build has failed. I have tried to fix the error in the test-on-node task, but can't seem to find where it comes from

Contributor

BraulioVM commented Mar 17, 2015

I'm not sure why the build has failed. I have tried to fix the error in the test-on-node task, but can't seem to find where it comes from

src/core/stacktrace.js
+ * @param {String|Error} error
+ * @return {String} error message
+ */
+function errorString ( error ) {

This comment has been minimized.

@jzaefferer

jzaefferer Mar 17, 2015

Member

If its used only in assert.js, yes, it should go there.

Also: Remove the space before the opening paren.

@jzaefferer

jzaefferer Mar 17, 2015

Member

If its used only in assert.js, yes, it should go there.

Also: Remove the space before the opening paren.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Mar 17, 2015

Member

Try grunt test-on-node --stack to get more details from grunt. Sadly the --stack option is still not enabled by default.

Member

jzaefferer commented Mar 17, 2015

Try grunt test-on-node --stack to get more details from grunt. Sadly the --stack option is still not enabled by default.

@BraulioVM

This comment has been minimized.

Show comment
Hide comment
@BraulioVM

BraulioVM Mar 17, 2015

Contributor

Done, thank you for the tip!

Now the build is passing in my local repository while it is not in jquery's. I think this may be browserstack's fault

Contributor

BraulioVM commented Mar 17, 2015

Done, thank you for the tip!

Now the build is passing in my local repository while it is not in jquery's. I think this may be browserstack's fault

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Mar 17, 2015

Member

Yeah, more BrowserStack timeouts. Haven't been able to track down the cause of those, so for now we have to just ignore them. I'll review the diff...

Member

jzaefferer commented Mar 17, 2015

Yeah, more BrowserStack timeouts. Haven't been able to track down the cause of those, so for now we have to just ignore them. I'll review the diff...

src/assert.js
+ return resultErrorString;
+ }
+}
+

This comment has been minimized.

@leobalter

leobalter Mar 17, 2015

Member

Not a specific rule, but as this is a support method, it should be placed in the end, or after the Assert declaration and prototype definition.

@leobalter

leobalter Mar 17, 2015

Member

Not a specific rule, but as this is a support method, it should be placed in the end, or after the Assert declaration and prototype definition.

This comment has been minimized.

@BraulioVM

BraulioVM Mar 17, 2015

Contributor

Yeah, it feels right at the end of the file.

I put it there because it was at the top of the core.js file, and thought it was something you guys preferred to do.

@BraulioVM

BraulioVM Mar 17, 2015

Contributor

Yeah, it feels right at the end of the file.

I put it there because it was at the top of the core.js file, and thought it was something you guys preferred to do.

@BraulioVM

This comment has been minimized.

Show comment
Hide comment
@BraulioVM

BraulioVM Mar 17, 2015

Contributor

I might have shaken things too much this time, specially with the control and loggingCallbacks directories.

Waiting to hear what you think of this.

Contributor

BraulioVM commented Mar 17, 2015

I might have shaken things too much this time, specially with the control and loggingCallbacks directories.

Waiting to hear what you think of this.

@BraulioVM

This comment has been minimized.

Show comment
Hide comment
@BraulioVM

BraulioVM Mar 18, 2015

Contributor

I have kept moving things out of core

The content of the new core/test.js file is suspicious to be refactored because it makes core depend on test, making it a circular dependency. I'll inspect this tomorrow.

Contributor

BraulioVM commented Mar 18, 2015

I have kept moving things out of core

The content of the new core/test.js file is suspicious to be refactored because it makes core depend on test, making it a circular dependency. I'll inspect this tomorrow.

@BraulioVM

This comment has been minimized.

Show comment
Hide comment
@BraulioVM

BraulioVM Mar 24, 2015

Contributor

The failed build is browserstack's fault again.

What do you guys think of the current state of the PR?

Contributor

BraulioVM commented Mar 24, 2015

The failed build is browserstack's fault again.

What do you guys think of the current state of the PR?

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Mar 24, 2015

Member

I have very limited bandwidth for PR reviews this week. I hope @leobalter can provide you with some feedback for now.

Member

jzaefferer commented Mar 24, 2015

I have very limited bandwidth for PR reviews this week. I hope @leobalter can provide you with some feedback for now.

Gruntfile.js
+ "src/core/control/resumeProcessing.js",
+ "src/core/control/done.js",
+ "src/core/module.js",
+ "src/core/test.js",

This comment has been minimized.

@leobalter

leobalter Mar 25, 2015

Member

This went deep, and I don't believe it's better to make this split into many files.

There're some things I liked, like the errorString function going to the assert.js file.

Maybe a better approach, using smaller files:

  • src/core/module.js: it doesn't exist.
  • src/core/test.js: may go to src/test.js
  • src/core/control/* they can stay at core.js
  • src/core/loggingCallbacks/* => src/core/logging.js would be fine
  • can create a src/utilities.js containing:
    • src/core/utilities.js
    • src/core/extend.js
    • src/core/objectType.js
    • src/core/urlParams.js
    • src/core/defined.js
  • src/core/config.js is fine
@leobalter

leobalter Mar 25, 2015

Member

This went deep, and I don't believe it's better to make this split into many files.

There're some things I liked, like the errorString function going to the assert.js file.

Maybe a better approach, using smaller files:

  • src/core/module.js: it doesn't exist.
  • src/core/test.js: may go to src/test.js
  • src/core/control/* they can stay at core.js
  • src/core/loggingCallbacks/* => src/core/logging.js would be fine
  • can create a src/utilities.js containing:
    • src/core/utilities.js
    • src/core/extend.js
    • src/core/objectType.js
    • src/core/urlParams.js
    • src/core/defined.js
  • src/core/config.js is fine
src/core/config.js
+var i,
+ config = {
+ // The queue of tests to run
+ queue: [],

This comment has been minimized.

@leobalter

leobalter Mar 25, 2015

Member

config object identation is wrong here.

@leobalter

leobalter Mar 25, 2015

Member

config object identation is wrong here.

+
+ callbacks: {}
+},
+ urlParams = getUrlParams();

This comment has been minimized.

@leobalter

leobalter Mar 25, 2015

Member

the src/core.js on line 10 there's a QUnit.urlParams = getUrlParams();, so you can replace this function call by urlParams = QUnit.urlParams; or actually remove that line from src/core and:

QUnit.urlParams = urlParams = getUrlParams();
@leobalter

leobalter Mar 25, 2015

Member

the src/core.js on line 10 there's a QUnit.urlParams = getUrlParams();, so you can replace this function call by urlParams = QUnit.urlParams; or actually remove that line from src/core and:

QUnit.urlParams = urlParams = getUrlParams();

This comment has been minimized.

@BraulioVM

BraulioVM Mar 29, 2015

Contributor

Would it be ok to remove the function call in src/core and instead make QUnit.urlParams = urlParams? I think it would be useful not to modify the global QUnit object from core/config, in order to make the codebase more modular

@BraulioVM

BraulioVM Mar 29, 2015

Contributor

Would it be ok to remove the function call in src/core and instead make QUnit.urlParams = urlParams? I think it would be useful not to modify the global QUnit object from core/config, in order to make the codebase more modular

This comment has been minimized.

src/core.js
- QUnit.version = "@VERSION";
-}());
+// Expose the current QUnit version
+QUnit.version = "@VERSION";
// Root QUnit object.
// `QUnit` initialized at top of scope

This comment has been minimized.

@jzaefferer

jzaefferer Mar 26, 2015

Member

No need for this comment anymore (there's another below that I'd also remove).

@jzaefferer

jzaefferer Mar 26, 2015

Member

No need for this comment anymore (there's another below that I'd also remove).

src/core.js
fileName = ( sourceFromStacktrace( 0 ) || "" ).replace( /(:\d+)+\)?/, "" ).replace( /.+\//, "" ),
- toString = Object.prototype.toString,
- hasOwn = Object.prototype.hasOwnProperty,
// Keep a local reference to Date (GH-283)

This comment has been minimized.

@jzaefferer

jzaefferer Mar 26, 2015

Member

Also outdated - either move along with the code it commented, or drop it.

@jzaefferer

jzaefferer Mar 26, 2015

Member

Also outdated - either move along with the code it commented, or drop it.

src/core.js
@@ -316,42 +111,9 @@ extend( QUnit, {
config: config,
// Safe object type checking

This comment has been minimized.

@jzaefferer

jzaefferer Mar 26, 2015

Member

Another comment that seems to be in the wrong place.

@jzaefferer

jzaefferer Mar 26, 2015

Member

Another comment that seems to be in the wrong place.

src/core.js
- }
-})();
+// Registers logging callbacks
+extend( QUnit, registerLoggingCallbacks() );

This comment has been minimized.

@jzaefferer

jzaefferer Mar 26, 2015

Member

Again, unnecessary comment. Could also refactor this a bit, e.g. registerLoggingCallbacks( QUnit ); - for that the function name makes more sense.

@jzaefferer

jzaefferer Mar 26, 2015

Member

Again, unnecessary comment. Could also refactor this a bit, e.g. registerLoggingCallbacks( QUnit ); - for that the function name makes more sense.

src/core/utilities.js
+ },
+ setTimeout = window.setTimeout,
+ clearTimeout = window.clearTimeout,
+ location = window.location || { search: "", protocol: "file:" };

This comment has been minimized.

@jzaefferer

jzaefferer Mar 26, 2015

Member

Most of these variables aren't used inside this file, that's a bad sign.

@jzaefferer

jzaefferer Mar 26, 2015

Member

Most of these variables aren't used inside this file, that's a bad sign.

@BraulioVM

This comment has been minimized.

Show comment
Hide comment
@BraulioVM

BraulioVM Mar 29, 2015

Contributor

I've been busy this week and thus not able to finish the PR. I'll try to get some work done very soon.

Contributor

BraulioVM commented Mar 29, 2015

I've been busy this week and thus not able to finish the PR. I'll try to get some work done very soon.

leobalter added a commit to leobalter/qunit that referenced this pull request Apr 9, 2015

Build: Remove Opera 12.1 from browserstack tests
Although the support is not over, the browserstack tests on that browser
is returning some random errors on Travis-CI, which might be due to
timeout errors, common sometimes, but with a misleading feedback.

Ref #785

leobalter added a commit that referenced this pull request Apr 9, 2015

Build: Remove Opera 12.1 from browserstack tests
Although the support is not over, the browserstack tests on that browser
is returning some random errors on Travis-CI, which might be due to
timeout errors, common sometimes, but with a misleading feedback.

Ref #785
Closes #809
@BraulioVM

This comment has been minimized.

Show comment
Hide comment
@BraulioVM

BraulioVM Apr 9, 2015

Contributor

Well, timeouts again.

Contributor

BraulioVM commented Apr 9, 2015

Well, timeouts again.

@BraulioVM

This comment has been minimized.

Show comment
Hide comment
@BraulioVM

BraulioVM Apr 16, 2015

Contributor

Any problem with this?

Contributor

BraulioVM commented Apr 16, 2015

Any problem with this?

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Apr 16, 2015

Member

No problem at all, just not enough time for all the PRs and issues. I'll take another look.

Member

leobalter commented Apr 16, 2015

No problem at all, just not enough time for all the PRs and issues. I'll take another look.

src/assert.js
+ * Based on http://es5.github.com/#x15.11.4.4
+ *
+ * @param {String|Error} error
+ * @return {String} error message

This comment has been minimized.

@leobalter

leobalter Apr 16, 2015

Member

we can remove the jsdoc styled comments.

@leobalter

leobalter Apr 16, 2015

Member

we can remove the jsdoc styled comments.

+ } else {
+ return resultErrorString;
+ }
+}

This comment has been minimized.

@leobalter

leobalter Apr 16, 2015

Member

missing eof line

@leobalter

leobalter Apr 16, 2015

Member

missing eof line

src/core.js
- QUnit[ key ] = registerLoggingCallback( key );
- }
-})();
+registerLoggingCallbacks(QUnit);

This comment has been minimized.

@leobalter

leobalter Apr 16, 2015

Member

missing spaces between arguments: registerLoggingCallbacks( QUnit );

@leobalter

leobalter Apr 16, 2015

Member

missing spaces between arguments: registerLoggingCallbacks( QUnit );

src/core.js
+ total: config.stats.all,
+ runtime: runtime
+ });
+}

This comment has been minimized.

@leobalter

leobalter Apr 16, 2015

Member

missing eof line

@leobalter

leobalter Apr 16, 2015

Member

missing eof line

+ for (var i = 0; i < urlParams.testId.length; i++ ) {
+ config.testId.push( urlParams.testId[ i ] );
+ }
+}

This comment has been minimized.

@leobalter

leobalter Apr 16, 2015

Member

missing eof line

@leobalter

leobalter Apr 16, 2015

Member

missing eof line

+ }
+
+ return extractStacktrace( error, offset );
+}

This comment has been minimized.

@leobalter

leobalter Apr 16, 2015

Member

missing eof line

@leobalter

leobalter Apr 16, 2015

Member

missing eof line

src/core/utilities.js
+ return false;
+ }
+ }())
+};

This comment has been minimized.

@leobalter

leobalter Apr 16, 2015

Member

missing eof line

@leobalter

leobalter Apr 16, 2015

Member

missing eof line

src/core/utilities.js
+ } catch ( e ) {
+ return false;
+ }
+ }())

This comment has been minimized.

@leobalter

leobalter Apr 16, 2015

Member

I'm not 100% sure but with the new style guide this should be }() )

cc @scottgonzalez, would you confirm this, please?

@leobalter

leobalter Apr 16, 2015

Member

I'm not 100% sure but with the new style guide this should be }() )

cc @scottgonzalez, would you confirm this, please?

src/test.js
+ if ( config.noglobals ) {
+ for ( var key in window ) {
+ if ( hasOwn.call( window, key ) ) {
+ // in Opera sometimes DOM element ids show up here, ignore them

This comment has been minimized.

@leobalter

leobalter Apr 16, 2015

Member

missing blank line above the comment

@leobalter

leobalter Apr 16, 2015

Member

missing blank line above the comment

This comment has been minimized.

@BraulioVM

BraulioVM Apr 18, 2015

Contributor

I am not sure I know what you mean. If I put a blank line above the comment, jscs will complain because of an illegal trailing whitespace.

@BraulioVM

BraulioVM Apr 18, 2015

Contributor

I am not sure I know what you mean. If I put a blank line above the comment, jscs will complain because of an illegal trailing whitespace.

This comment has been minimized.

@jzaefferer

jzaefferer Apr 19, 2015

Member

Maybe you ended up with whitespace on that blank line? There should be only a line break, no spaces or tabs.

@jzaefferer

jzaefferer Apr 19, 2015

Member

Maybe you ended up with whitespace on that blank line? There should be only a line break, no spaces or tabs.

This comment has been minimized.

@BraulioVM

BraulioVM Apr 20, 2015

Contributor

That was the problem!

Thanks for the tip

@BraulioVM

BraulioVM Apr 20, 2015

Contributor

That was the problem!

Thanks for the tip

+ });
+
+ test.queue();
+}

This comment has been minimized.

@leobalter

leobalter Apr 16, 2015

Member

missing eof line

@leobalter

leobalter Apr 16, 2015

Member

missing eof line

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Apr 16, 2015

Member

caught only code style issues, and I'm already running tests on a local updated branch and I'll also manually check browserstack.

Member

leobalter commented Apr 16, 2015

caught only code style issues, and I'm already running tests on a local updated branch and I'll also manually check browserstack.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Apr 16, 2015

Member

I'm already running tests on a local updated branch and I'll also manually check browserstack.

Passing!

Member

leobalter commented Apr 16, 2015

I'm already running tests on a local updated branch and I'll also manually check browserstack.

Passing!

BraulioVM added some commits Apr 18, 2015

@BraulioVM

This comment has been minimized.

Show comment
Hide comment
@BraulioVM

BraulioVM Apr 20, 2015

Contributor

Ok, everything has been fixed! Is this ready for merge?

Contributor

BraulioVM commented Apr 20, 2015

Ok, everything has been fixed! Is this ready for merge?

@BraulioVM BraulioVM changed the title from Split core.js into smaller chunks [WIP] to Split core.js into smaller chunks Apr 22, 2015

@BraulioVM

This comment has been minimized.

Show comment
Hide comment
@BraulioVM

BraulioVM Apr 22, 2015

Contributor

I have changed the title of the PR because it seems to be ready. Any other thing I should do?

Contributor

BraulioVM commented Apr 22, 2015

I have changed the title of the PR because it seems to be ready. Any other thing I should do?

@BraulioVM

This comment has been minimized.

Show comment
Hide comment
@BraulioVM

BraulioVM Apr 25, 2015

Contributor

Any thoughts?

Contributor

BraulioVM commented Apr 25, 2015

Any thoughts?

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer May 15, 2015

Member

By now this needs a rebase to be able to merge it. Haven't yet looked into the individual conflicts.

Member

jzaefferer commented May 15, 2015

By now this needs a rebase to be able to merge it. Haven't yet looked into the individual conflicts.

@BraulioVM

This comment has been minimized.

Show comment
Hide comment
@BraulioVM

BraulioVM Jun 5, 2015

Contributor

Sorry I have been afk because I had to study for my exams. Still any interest?

Contributor

BraulioVM commented Jun 5, 2015

Sorry I have been afk because I had to study for my exams. Still any interest?

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Jun 5, 2015

Member

Yes, sure. I'm actually you're back!

On Jun 5, 2015, at 1:49 PM, Braulio Valdivielso Martínez notifications@github.com wrote:

Sorry I have been afk because I had to study for my exams. Still any interest?


Reply to this email directly or view it on GitHub.

Member

leobalter commented Jun 5, 2015

Yes, sure. I'm actually you're back!

On Jun 5, 2015, at 1:49 PM, Braulio Valdivielso Martínez notifications@github.com wrote:

Sorry I have been afk because I had to study for my exams. Still any interest?


Reply to this email directly or view it on GitHub.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Jun 17, 2015

Member

I think Leo meant "glad you're back". @BraulioVM do you need more input somewhere? Seems like for now we "just" need a rebase.

Member

jzaefferer commented Jun 17, 2015

I think Leo meant "glad you're back". @BraulioVM do you need more input somewhere? Seems like for now we "just" need a rebase.

@BraulioVM

This comment has been minimized.

Show comment
Hide comment
@BraulioVM

BraulioVM Jun 17, 2015

Contributor

I'm sorry it took me so long but I'm studying for my exams right now and couldn't look into this.

I hope it's done now. Tell me if anything else is needed, but I cannot assure you whether I will be able to get any important work done as I'm still studying.

Contributor

BraulioVM commented Jun 17, 2015

I'm sorry it took me so long but I'm studying for my exams right now and couldn't look into this.

I hope it's done now. Tell me if anything else is needed, but I cannot assure you whether I will be able to get any important work done as I'm still studying.

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Jun 17, 2015

Member

Yes, I'm glad. My autocorrector is not because of multiple languages everyday.

On Jun 17, 2015, at 10:28 AM, Jörn Zaefferer notifications@github.com wrote:

I think Leo meant "glad you're back". @BraulioVM do you need more input somewhere? Seems like for now we "just" need a rebase.


Reply to this email directly or view it on GitHub.

Member

leobalter commented Jun 17, 2015

Yes, I'm glad. My autocorrector is not because of multiple languages everyday.

On Jun 17, 2015, at 10:28 AM, Jörn Zaefferer notifications@github.com wrote:

I think Leo meant "glad you're back". @BraulioVM do you need more input somewhere? Seems like for now we "just" need a rebase.


Reply to this email directly or view it on GitHub.

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Jun 18, 2015

Member

We really need to land this as soon as possible. Going through my email notifications in chronological order this morning, I ended up merging #822 before looking at this, and that caused another merge conflict. Sorry about that.

@BraulioVM could you do the rebase magic once more? I think we can manage to hold off on any other commits to master for a little while, until this is done. Hopefully you can squeeze this in between your study.

Member

jzaefferer commented Jun 18, 2015

We really need to land this as soon as possible. Going through my email notifications in chronological order this morning, I ended up merging #822 before looking at this, and that caused another merge conflict. Sorry about that.

@BraulioVM could you do the rebase magic once more? I think we can manage to hold off on any other commits to master for a little while, until this is done. Hopefully you can squeeze this in between your study.

@BraulioVM

This comment has been minimized.

Show comment
Hide comment
@BraulioVM

BraulioVM Jun 18, 2015

Contributor

Rebase magic done.

CI is failing, but I think it is not my fault. Could you look into it? It looks like there is a bug in browserstack/cli.js

Contributor

BraulioVM commented Jun 18, 2015

Rebase magic done.

CI is failing, but I think it is not my fault. Could you look into it? It looks like there is a bug in browserstack/cli.js

@jzaefferer

This comment has been minimized.

Show comment
Hide comment
@jzaefferer

jzaefferer Jul 15, 2015

Member

@leobalter could you do another review of this?

Member

jzaefferer commented Jul 15, 2015

@leobalter could you do another review of this?

@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter

leobalter Jul 15, 2015

Member

LGTM, :shipit:

Member

leobalter commented Jul 15, 2015

LGTM, :shipit:

leobalter added a commit that referenced this pull request Jul 15, 2015

@leobalter leobalter closed this in 825691f Jul 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment