Split core.js into smaller chunks by BraulioVM · Pull Request #785 · qunitjs/qunit

BraulioVM · 2015-03-17T00:19:51Z

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 · 2015-03-17T00:20:34Z

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

jzaefferer · 2015-03-17T10:36:17Z

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

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

BraulioVM · 2015-03-17T11:32:55Z

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

jzaefferer · 2015-03-17T12:16:14Z

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

Also: Remove the space before the opening paren.

jzaefferer · 2015-03-17T12:19:03Z

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

BraulioVM · 2015-03-17T13:07:30Z

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 · 2015-03-17T13:18:58Z

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...

leobalter · 2015-03-17T17:12:37Z

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.

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 · 2015-03-17T20:51:39Z

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 · 2015-03-18T23:56:32Z

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 · 2015-03-24T22:29:56Z

The failed build is browserstack's fault again.

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

jzaefferer · 2015-03-24T22:48:32Z

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

leobalter · 2015-03-25T15:23:52Z

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 · 2015-04-16T14:36:56Z

missing eof line

leobalter · 2015-04-16T14:42:35Z

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

leobalter · 2015-04-16T15:58:12Z

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

Passing!

BraulioVM · 2015-04-20T13:20:01Z

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

BraulioVM · 2015-04-22T08:53:45Z

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

BraulioVM · 2015-04-25T11:35:07Z

Any thoughts?

jzaefferer · 2015-05-15T16:59:35Z

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

BraulioVM · 2015-06-05T17:49:43Z

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

leobalter · 2015-06-05T18:04:13Z

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 · 2015-06-17T14:28:15Z

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 · 2015-06-17T22:16:04Z

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 · 2015-06-17T22:38:09Z

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 · 2015-06-18T11:03:55Z

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 · 2015-06-18T12:43:40Z

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 · 2015-07-15T11:43:39Z

@leobalter could you do another review of this?

leobalter · 2015-07-15T16:46:39Z

LGTM,

Closes #785 Fixes #782

BraulioVM added 2 commits March 16, 2015 21:31

Core: Extracted config, urlParams, utilities and extend

7e9656a

Core: extracted stacktrace and objectType

b039d30

jquerybot added the CLA: Valid label Mar 17, 2015

BraulioVM added 2 commits March 17, 2015 01:22

Core: removed unuseful comment

81b3be3

Core: extracted utility functions and the callback registration routine

a1cf6b7

jzaefferer reviewed Mar 17, 2015
View reviewed changes

BraulioVM added 3 commits March 17, 2015 12:14

Core: fixed typo, login => logging

4e184dd

Merge branch 'master' of https://github.com/jquery/qunit into split-core

64081e4

Core: moved errorString to stacktrace.js

cc20e28

jzaefferer reviewed Mar 17, 2015
View reviewed changes

Core: fixed location bug, not defined in node

d8b54d8

Core: moved errorString => assert

2381938

leobalter reviewed Mar 17, 2015
View reviewed changes

BraulioVM added 4 commits March 17, 2015 20:50

Assert: moved errorString to the end of the file

fc24558

Core: put all loggingCallback logic inside their own directory

a8edb03

Core: moved synchronize to the only file it's used by

fdbb0ef

Core: moved things around

0c327a5

Core: took test, asyncTest and skip out of core.js

e489e57

Core: fixed previous commit

2b3197f

leobalter reviewed Mar 25, 2015
View reviewed changes

leobalter reviewed Apr 16, 2015
View reviewed changes

Comment thread src/core.js Outdated

leobalter Apr 16, 2015

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing eof line

BraulioVM added 2 commits April 18, 2015 13:49

Core: fixed style issues

acc7333

Test: fixed style issues

bdb319f

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

Merge branch 'master' of https://github.com/jquery/qunit into split-core

e613cde

Merge branch 'master' of https://github.com/jquery/qunit into split-core

2f23bab

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

Build: Split source to smaller files.

0bb2375

Closes #785 Fixes #782

leobalter closed this in 825691f Jul 15, 2015

Conversation

BraulioVM commented Mar 17, 2015

Uh oh!

BraulioVM commented Mar 17, 2015

Uh oh!

jzaefferer Mar 17, 2015

Choose a reason for hiding this comment

Uh oh!

BraulioVM Mar 17, 2015

Choose a reason for hiding this comment

Uh oh!

BraulioVM commented Mar 17, 2015

Uh oh!

jzaefferer Mar 17, 2015

Choose a reason for hiding this comment

Uh oh!

jzaefferer commented Mar 17, 2015

Uh oh!

BraulioVM commented Mar 17, 2015

Uh oh!

jzaefferer commented Mar 17, 2015

Uh oh!

leobalter Mar 17, 2015

Choose a reason for hiding this comment

Uh oh!

BraulioVM Mar 17, 2015

Choose a reason for hiding this comment

Uh oh!

BraulioVM commented Mar 17, 2015

Uh oh!

BraulioVM commented Mar 18, 2015

Uh oh!

BraulioVM commented Mar 24, 2015

Uh oh!

jzaefferer commented Mar 24, 2015

Uh oh!

leobalter Mar 25, 2015

Choose a reason for hiding this comment

Uh oh!

leobalter Apr 16, 2015

Choose a reason for hiding this comment

Uh oh!

leobalter commented Apr 16, 2015

Uh oh!

leobalter commented Apr 16, 2015

Uh oh!

BraulioVM commented Apr 20, 2015

Uh oh!

BraulioVM commented Apr 22, 2015

Uh oh!

BraulioVM commented Apr 25, 2015

Uh oh!

jzaefferer commented May 15, 2015

Uh oh!

BraulioVM commented Jun 5, 2015

Uh oh!

leobalter commented Jun 5, 2015

Uh oh!

jzaefferer commented Jun 17, 2015

Uh oh!

BraulioVM commented Jun 17, 2015

Uh oh!

leobalter commented Jun 17, 2015

Uh oh!

jzaefferer commented Jun 18, 2015

Uh oh!

BraulioVM commented Jun 18, 2015

Uh oh!

jzaefferer commented Jul 15, 2015

Uh oh!

leobalter commented Jul 15, 2015

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants