Code cleanup #120

BehindTheMath · 2018-01-30T05:01:06Z

Code cleanup
Cleanup parse-options tests
Clean up README
Remove old switchFallback code

robinnorth

All the proposed changes look good to me!

There are a few more long-standing code comments that could possibly do with removing as part of a cleanup too -- things like commented-out module require calls and disabled experimental code. https://github.com/MoOx/pjax/blob/master/lib/switches.js has a lot of examples of this. https://github.com/MoOx/pjax/blob/master/lib/switches-selectors.js#L15-L20 and https://github.com/MoOx/pjax/blob/master/index.js#L12 also spring to mind.

Finally, do we still need the polyfill for Function.prototype.bind? AFAIK it's supported on all platforms that also fully support the History API.

BehindTheMath · 2018-01-30T14:42:12Z

I'll take a look at those later.

BTW, you should also be able to push commits to this PR. Just checkout upstream/cleanup/misc-cleanup, and then push your commits back to the upstream branch.

robinnorth · 2018-01-30T17:10:05Z

Sure, thanks! I made comments rather than commits to avoid stepping on your toes, but if you like, I can action the changes I suggested. Equally happy to leave them to you, if you like.

BehindTheMath · 2018-01-31T21:22:41Z

Everything looks good, but I think we should leave this open until we're ready to release 0.2.5, in case there's anything we want to add.

robinnorth · 2018-01-31T21:44:39Z

Sounds good to me, hopefully we can get 0.2.5 out fairly soon!

- Rename objects for clarity and inline unneeded objects - Remove unneeded tests - Use Object.keys().length instead of a custom function - Use typeof === "object" instead of a custom function that checks the prototype tree as well, since we don't expect anything but an object literal.

robinnorth · 2018-01-31T22:31:21Z

I've actioned the items I suggested previously and rebased against master to keep this PR up-to-date. I can't think of anything else that needs cleaning up at present, other than possibly the description in package.json (and adding a contributors attribute?).

BehindTheMath · 2018-02-01T02:19:45Z

lib/eval-script.js

-    script.async = false; // force synchronous loading of peripheral js
+  if (src !== "") {
+    script.src = src
+    script.async = false // force asynchronous loading of peripheral js


@robinnorth I'm confused. Why did you change this back? I thought we agreed it was synchronous.

Oops, I guess I didn't quite successfully resolve that merge conflict... Will fix.

MoOx · 2018-02-01T06:53:09Z

(I think it's probably time for 1.0 ;) )

BehindTheMath · 2018-02-01T18:59:22Z

@robinnorth What do you think about that last commit (2ec4c14)?

Closes #17

BehindTheMath · 2018-02-01T19:04:30Z

It looks like after this PR is merged, all issues milestoned for 0.2.5 will be closed.

Is there anything else that needs to be done before working on releasing 0.2.5?

robinnorth · 2018-02-01T20:20:51Z

@robinnorth What do you think about that last commit (2ec4c14)?

The library was obviously modularised with testability in mind, but these modules have never had corresponding tests, so it's probably OK to inline them like this.

Is there anything else that needs to be done before working on releasing 0.2.5?

Only a version bump and any changes to package.json (I added you and I as contributors, hope that's OK) and then we're good to go! Do you have npm publish access?

BehindTheMath · 2018-02-01T20:28:44Z

I added you and I as contributors, hope that's OK

@MoOx Should we add everyone who contributed (https://github.com/MoOx/pjax/graphs/contributors)?

Do you have npm publish access

I do not. I'll update the changelog and version number, and then @MoOx can publish it.

BehindTheMath · 2018-02-01T21:37:21Z

The library was obviously modularised with testability in mind

With that in mind, should we move out loadUrl(), loadContent(), and afterAllSwitches() to separate files?

robinnorth · 2018-02-01T21:59:02Z

With that in mind, should we move out loadUrl(), loadContent(), and afterAllSwitches() to separate files?

It would be great to modularise and be able to independently test these methods, as they're basically the core of the whole library, but it's probably going to be a bit of an undertaking to do so right now. I think we should get this next release out and then add this as an aim for the next major milestone (and close #21 in the process).

BehindTheMath · 2018-02-01T22:02:34Z

Sounds good.

So we're just waiting on @MoOx for an answer about other contributors, and then we can merge this. I have a branch with the updated changelog and the version bump that I'll rebase and push once this is merged.

MoOx · 2018-02-02T05:17:29Z

Do whatever you feel appropriate.
I can give you npm access if you want or I can release it too. Just tell me what you prefer.

To make changelog easier I would recommend you in the future to only use "Squash and merge" option when merging PR. That helps to have a clean git history with kind of "one pr = one feature or a bugfix" history, which is easy to review and create release notes from.

BehindTheMath · 2018-02-02T05:23:23Z

I can give you npm access if you want or I can release it too. Just tell me what you prefer.

If you can give me access, we won't have to bother you in the future. I'll DM you on Twitter.

To make changelog easier I would recommend you in the future to only use "Squash and merge" option when merging PR. That helps to have a clean git history with kind of "one pr = one feature or a bugfix" history, which is easy to review and create release notes from.

I wasn't sure which to use, since it looks like in the past sometimes merge commits were used instead of squashing. Also, using merge commits doesn't require as much rebasing. But we can start squashing.

MoOx · 2018-02-02T05:36:03Z

You have been added to npm ;)

For the merging, it's a recommendation. At the time I made this lib, not sure it was a thing :D

BehindTheMath · 2018-02-02T05:38:02Z

You have been added to npm ;)

Great, thanks.

Do whatever you feel appropriate.

@robinnorth What do you think? Should we add anyone else?

robinnorth · 2018-02-02T09:15:49Z

@BehindTheMath it's up to you -- we could add/ask to add anyone who has ever made a contribution via a PR and add a note encouraging future contributors to add themselves to the list when making a PR, or we could just list contributors who have made a significant contribution (i.e. multiple PRs)/have commit access to the repo, or leave it as is. My personal inclination is that one PR doesn't really qualify for 'contributor' status, but you were here before me, so I'll defer to you on this 😄

BehindTheMath · 2018-02-02T14:49:14Z

I don't want to slight anyone by leaving them out, but at the same time I think it will get unwieldy if there's a huge list of everyone who submitted commits.

I'm going to leave it as it is, and if anyone wants, they can add themselves in.

robinnorth · 2018-02-02T15:44:03Z

I think that's a sensible approach 👍

BehindTheMath requested a review from robinnorth January 30, 2018 05:01

robinnorth reviewed Jan 30, 2018

View reviewed changes

BehindTheMath and others added 6 commits January 31, 2018 22:19

Code cleanup

6fb509a

Clean up README

70f6383

Remove old switchFallback code

5ec0ce0

More README cleanup

9971c4c

Code style cleanup, remove redundant comments

1eb43f7

robinnorth force-pushed the cleanup/misc-cleanup branch from 4f6690a to 1eb43f7 Compare January 31, 2018 22:21

Remove polyfill for Function.prototype.bind

258d66f

BehindTheMath commented Feb 1, 2018

View reviewed changes

robinnorth and others added 3 commits February 1, 2018 08:45

Correctly resolve rebase conflict

a893dfd

More comments cleanup

d0d3081

Inline small functions

2ec4c14

Add more documentation and tests for options.currentUrlFullReload

a2aced4

Closes #17

BehindTheMath and others added 2 commits February 1, 2018 14:07

Fix error with 2ec4c14

b3a20c3

Update package.json

891b77b

BehindTheMath merged commit a72880d into master Feb 2, 2018

BehindTheMath deleted the cleanup/misc-cleanup branch February 2, 2018 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code cleanup #120

Code cleanup #120

BehindTheMath commented Jan 30, 2018

robinnorth left a comment

BehindTheMath commented Jan 30, 2018

robinnorth commented Jan 30, 2018

BehindTheMath commented Jan 31, 2018

robinnorth commented Jan 31, 2018

robinnorth commented Jan 31, 2018

BehindTheMath Feb 1, 2018

robinnorth Feb 1, 2018

MoOx commented Feb 1, 2018

BehindTheMath commented Feb 1, 2018

BehindTheMath commented Feb 1, 2018 •

edited

Loading

robinnorth commented Feb 1, 2018

BehindTheMath commented Feb 1, 2018

BehindTheMath commented Feb 1, 2018

robinnorth commented Feb 1, 2018

BehindTheMath commented Feb 1, 2018

MoOx commented Feb 2, 2018

BehindTheMath commented Feb 2, 2018

MoOx commented Feb 2, 2018

BehindTheMath commented Feb 2, 2018

robinnorth commented Feb 2, 2018

BehindTheMath commented Feb 2, 2018 •

edited

Loading

robinnorth commented Feb 2, 2018

Code cleanup #120

Code cleanup #120

Conversation

BehindTheMath commented Jan 30, 2018

robinnorth left a comment

Choose a reason for hiding this comment

BehindTheMath commented Jan 30, 2018

robinnorth commented Jan 30, 2018

BehindTheMath commented Jan 31, 2018

robinnorth commented Jan 31, 2018

robinnorth commented Jan 31, 2018

BehindTheMath Feb 1, 2018

Choose a reason for hiding this comment

robinnorth Feb 1, 2018

Choose a reason for hiding this comment

MoOx commented Feb 1, 2018

BehindTheMath commented Feb 1, 2018

BehindTheMath commented Feb 1, 2018 • edited Loading

robinnorth commented Feb 1, 2018

BehindTheMath commented Feb 1, 2018

BehindTheMath commented Feb 1, 2018

robinnorth commented Feb 1, 2018

BehindTheMath commented Feb 1, 2018

MoOx commented Feb 2, 2018

BehindTheMath commented Feb 2, 2018

MoOx commented Feb 2, 2018

BehindTheMath commented Feb 2, 2018

robinnorth commented Feb 2, 2018

BehindTheMath commented Feb 2, 2018 • edited Loading

robinnorth commented Feb 2, 2018

BehindTheMath commented Feb 1, 2018 •

edited

Loading

BehindTheMath commented Feb 2, 2018 •

edited

Loading