Attempt to clean things up some #4193

unquietwiki · 2018-05-04T23:10:00Z

Hey @jashkenas . I've been working on a ES5->ES2015 project conversion; and have stumbled into trying to cleanup & assure that the assorted dependencies are reliable. I already have a pull in with Mediator; I figured this should get some similar attention. If this works, maybe it could be the starting point to resolve tickets like #3560 & #3924

Thanks!

unquietwiki · 2018-05-04T23:25:24Z

I snipped out the relevant failed tests from the Travis log

PhantomJS 2.1.1 (Linux 0.0.0) Backbone.Collection preinitialize FAILED
	Expected: 1
	Actual: undefined
	test/collection.js:672:17
	
PhantomJS 2.1.1 (Linux 0.0.0) Backbone.Collection preinitialize occurs before the collection is set up FAILED
	Expected 2 assertions, but 1 were run
	test/collection.js:675:13
	global code@test/collection.js:2103:3
	
PhantomJS 2.1.1 (Linux 0.0.0) Backbone.Collection mixin FAILED
	Died on test #1 test/collection.js:714:13
	global code@test/collection.js:2103:3: undefined is not a function (evaluating 'Backbone.Collection.mixin')
	test/collection.js:715:30
	
PhantomJS 2.1.1 (Linux 0.0.0) Backbone.Collection Collection.values iterates models in sorted order FAILED
	Died on test #1 test/collection.js:1797:13
	global code@test/collection.js:2103:3: undefined is not a function (evaluating 'collection.values()')
	test/collection.js:1803:37
	
	Expected 4 assertions, but 1 were run
	test/collection.js:1797:13
	global code@test/collection.js:2103:3
	
PhantomJS 2.1.1 (Linux 0.0.0) Backbone.Collection Collection.keys iterates ids in sorted order FAILED
	Died on test #1 test/collection.js:1810:13
	global code@test/collection.js:2103:3: undefined is not a function (evaluating 'collection.keys()')
	test/collection.js:1816:35
	
	Expected 4 assertions, but 1 were run
	test/collection.js:1810:13
	global code@test/collection.js:2103:3
	
PhantomJS 2.1.1 (Linux 0.0.0) Backbone.Collection Collection.entries iterates ids and models in sorted order FAILED
	Died on test #1 test/collection.js:1823:13
	global code@test/collection.js:2103:3: undefined is not a function (evaluating 'collection.entries()')
	test/collection.js:1829:38
	
	Expected 4 assertions, but 1 were run
	test/collection.js:1823:13
	global code@test/collection.js:2103:3
	
PhantomJS 2.1.1 (Linux 0.0.0) Backbone.Collection get models with `attributes` key FAILED
	failed, expected argument to be truthy, was: undefined
	Expected: true
	Actual: undefined
	test/collection.js:2101:14
	
PhantomJS 2.1.1 (Linux 0.0.0) Backbone.Events #3611 - listenTo is compatible with non-Backbone event libraries FAILED
	Died on test #1 test/events.js:706:13
	global code@test/events.js:743:3: undefined is not a function (evaluating 'this.events[name]()')
	trigger@test/events.js:714:26
	test/events.js:719:18

PhantomJS 2.1.1 (Linux 0.0.0) Backbone.Events #3611 - stopListening is compatible with non-Backbone event libraries FAILED
	Expected: 0
	Actual: 1
	test/events.js:741:17

PhantomJS 2.1.1 (Linux 0.0.0) Backbone.Model preinitialize FAILED
	Expected: 1
	Actual: undefined
	test/model.js:76:17
	
PhantomJS 2.1.1 (Linux 0.0.0) Backbone.Model preinitialize occurs before the model is set up FAILED
	Expected 6 assertions, but 3 were run
	test/model.js:80:13
	global code@test/model.js:1470:3
	
PhantomJS 2.1.1 (Linux 0.0.0) Backbone.Model mixin FAILED
	Died on test #1 test/model.js:1398:13
	global code@test/model.js:1470:3: undefined is not a function (evaluating 'Backbone.Model.mixin({
	      isEqual: function(model1, model2) {
	        return _.isEqual(model1, model2.attributes);
	      }
	    })')
	test/model.js:1399:25

	PhantomJS 2.1.1 (Linux 0.0.0) Backbone.Router preinitialize FAILED
	Expected: "foo"
	Actual: undefined
	test/router.js:191:17
	
PhantomJS 2.1.1 (Linux 0.0.0) Backbone.Router #4025 - navigate updates URL hash as is FAILED
	Expected: "#search/has%20space"
	Actual: "#search/has space"
	test/router.js:1078:23
	
PhantomJS 2.1.1 (Linux 0.0.0) Backbone.View preinitialize FAILED
	Expected: 1
	Actual: undefined
	test/view.js:72:23
	
PhantomJS 2.1.1 (Linux 0.0.0) Backbone.View preinitialize occurs before the view is set up FAILED
	Expected 2 assertions, but 1 were run
	test/view.js:75:13
	global code@test/view.js:516:3

jashkenas · 2018-05-04T23:47:56Z

It looks like there are a lot of unrelated automatic style changes in this PR. Don't you think the meaningful changes should be separated out from stylistic things to make this easier to assess?

unquietwiki · 2018-05-05T00:11:21Z

Hey there. So the goal here was to use StandardJS rules to make the code more readable; and identify basic errors. The other major change was fixing up the testing system to account for newer browsers & NodeJS. This allowed for the creation of the bug report. On May 4, 2018 4:48 PM, "Jeremy Ashkenas" <notifications@github.com> wrote: It looks like there are a lot of unrelated automatic style changes in this PR. Don't you think the meaningful changes should be separated out from stylistic things to make this easier to assess? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4193 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA9fv1TGzGOG269_SVTA9IizxCaIwKkCks5tvOjHgaJpZM4TzYR1> .

paulfalgout

I would also suggest doing this in smaller chunks. Mainly though we should avoiding reverting every change since 1.3.3

paulfalgout · 2018-05-05T05:47:41Z

bower.json

@@ -2,7 +2,7 @@
  "name"          : "backbone",
  "main"          : "backbone.js",
  "dependencies"  : {
-    "underscore"  : ">=1.8.3"
+    "underscore"  : ">=1.9.0"


did something change that makes 1.8.3 not still valid?

I know he updated Underscore recently, and there are still bugs over in that being looked at. I was just going over unit test code when you replied! (too tired to finish that up tonight)

Why does it matter if underscore was updated? Currently this lib support 1.8.3 and 1.9.x etc.. upping this to 1.9.0 removes support for 1.8.3 unnecessarily. Unless you've used something from 1.9 that isn't in 1.8.3?

Well, if the rule before was ^1.8.3, it's gonna go to 1.9.0 anyway on an npm install.

I need sleep. I can go over this stuff later again.

Yes but if your rule was 1.8.3 you are out of luck

It is also worth noting that backbone versions are not semver

paulfalgout · 2018-05-05T05:49:45Z

package.json

    "doc": "docco backbone.js && docco examples/todos/todos.js examples/backbone.localStorage.js",
    "lint": "eslint backbone.js test/*.js"
  },
  "main": "backbone.js",
-  "version": "1.3.3",
+  "version": "1.3.4",


there's no reason to assume this would be 1.3.4

I didn't want it getting confused with the current version.

paulfalgout · 2018-05-05T05:50:44Z

backbone.js

    options || (options = {});
-    this.preinitialize.apply(this, arguments);


this is removing all of the changes currently in master

I forked from master. What did I miss????

paulfalgout · 2018-05-05T05:51:32Z

backbone.js

      return this;
    },

    // Remove this view by taking the element out of the DOM, and removing any
    // applicable Backbone.Events listeners.
-    remove: function() {
+    remove: function () {


I think this rule should change. Linting is great, but there's no need to modify the current style if there is already a precedent.

Please edit the .eslintrc with the desired spec. My primary concern is bug-hunting, followed by figuring out how best to make sure this & underscore are working, so I know my efforts on floorplan aren't in vain.

unquietwiki · 2018-05-07T17:16:04Z

@paulfalgout Good morning! I dunno if you saw it, but over the weekend I did try to address your concerns regarding the PR. I hope they helped. Sorry if I wasn't as compliant Friday night: I was exhausted.

unquietwiki · 2018-05-07T17:35:29Z

@jashkenas @paulfalgout Whether this particular PR gets rejected / reworked / whatnot; I do have interest in getting some of these libraries updated to ES2015+ syntax. I've had to crash-course getting up to speed on JS in the past few months; and this is right when Mozilla is finishing up some key ES2015+ support for public use this week. The app I've been working on is 5+ years old (pre-2015); and the things I've found in trying to make that work, appear to solve a lot of old problems. I view such effort not unlike office filing: tedious, but necessary. Babel & Browserfy also seem useful in allowing for the source to be ES2015+; but still have production builds that'll run on ES5 runtimes.

unquietwiki · 2018-05-11T19:16:32Z

@paulfalgout @jashkenas I'll close this PR & do a revised one as able. I figure you might appreciate one that has the unit testing cleaned up, but the base code "virgin" to be less disruptive? Due apologies for any trouble on all of this.

unquietwiki added 3 commits May 4, 2018 15:47

Attempt at 1.3.4

ba72a1f

Followup on previous publish

b27be5f

Fix travis

ce9f164

unquietwiki mentioned this pull request May 5, 2018

Mimicked effort from Backbone PR 4193 jashkenas/underscore#2754

Closed

paulfalgout requested changes May 5, 2018

View reviewed changes

unquietwiki added 4 commits May 5, 2018 09:22

Attempt to address linting concerns

9f18efd

Forgot to fix bower per reviewer flag

0c6c191

New build per reviewer requests

7c4d907

Fixed EOL; some test fixes & formatting

c42f4b8

unquietwiki closed this May 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attempt to clean things up some #4193

Attempt to clean things up some #4193

unquietwiki commented May 4, 2018

unquietwiki commented May 4, 2018

jashkenas commented May 4, 2018

unquietwiki commented May 5, 2018 via email

paulfalgout left a comment

paulfalgout May 5, 2018

unquietwiki May 5, 2018

paulfalgout May 5, 2018 •

edited

unquietwiki May 5, 2018

paulfalgout May 5, 2018

paulfalgout May 5, 2018

paulfalgout May 5, 2018

unquietwiki May 5, 2018

paulfalgout May 5, 2018

unquietwiki May 5, 2018

paulfalgout May 5, 2018

unquietwiki May 5, 2018

unquietwiki commented May 7, 2018

unquietwiki commented May 7, 2018

unquietwiki commented May 11, 2018

		options \|\| (options = {});
		this.preinitialize.apply(this, arguments);

Attempt to clean things up some #4193

Attempt to clean things up some #4193

Conversation

unquietwiki commented May 4, 2018

unquietwiki commented May 4, 2018

jashkenas commented May 4, 2018

unquietwiki commented May 5, 2018 via email

paulfalgout left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulfalgout May 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unquietwiki commented May 7, 2018

unquietwiki commented May 7, 2018

unquietwiki commented May 11, 2018

paulfalgout May 5, 2018 •

edited