Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

LocationHistory rework proposal #687

Closed
wants to merge 6 commits into from
Closed

LocationHistory rework proposal #687

wants to merge 6 commits into from

Conversation

Goldob
Copy link
Contributor

@Goldob Goldob commented Jun 28, 2016

Relates with #653.

This is really just #656 after rebase. I decided to set up a new PR because Travis stopped responding to new commits and his output is crucial to illustrate the point (feel free to take a look at the unit tests).

With the proposed changes, a procedure for transitioning from old to new page will work as follows:

  1. Call new.onbeforeload and wait for callback1
  2. Mark new as current page
  3. Call old.onafterunload2

1 This was not always the case in the old version
2 I decided it is more suitable than onbeforeunload, because when we clean view-related variables, we want to do it after we stop rendering the page (or we risk errors otherwise). Also, this eliminates redundant complexity from having two onbefores - e.g. what if one has completed successfully, but the other returned an error? Do we just quietly abort the page change? But then the view whose function worked properly doesn't even know it's not being (un)loaded anymore. Maybe we should call the opposing function to notify it? What if it returns an error? You get the point.

Goldob added 4 commits June 28, 2016 20:59
Transition old -> new page (via go, next or back method):
1. call new.onbeforeload and wait for callback
2. replace old with new as the current page
3. call old.onafterunload
@Goldob
Copy link
Contributor Author

Goldob commented Jul 5, 2016

@dcposch @mathiasvr

@mathiasvr
Copy link
Contributor

LGTM, but why do we need babel?

@@ -0,0 +1,176 @@
/* eslint-env mocha */
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not need this because we are using standard.js?

Copy link
Contributor Author

@Goldob Goldob Jul 5, 2016

Choose a reason for hiding this comment

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

Yes. Mocha uses global functions, which cause standard.js to raise '{...}' is not defined errors.

@Goldob
Copy link
Contributor Author

Goldob commented Jul 5, 2016

LGTM, but why do we need babel?

Actually, we don't. Thanks for pointing that out.

Mocha threw errors on legit code and I thought it had some problems with ES6. However, as it turns out, adding --use_strict flag to mocha is enough to fix that.

Add --use_strict flag to Mocha instead of compiling with Babel
@dcposch
Copy link
Contributor

dcposch commented Jul 8, 2016

this looks very reasonable to me. thanks for adding tests for LocationHistory---they look thorough!

webtorrent desktop hasn't had any tests so far. that was fine for prototyping. now that the app has good number of users (and features) i think it's worth the extra effort to make sure nothing breaks.

two questions--not for this PR specifically but for WebTorrent Desktop in general

  • do we want unit tests or only integration tests
  • do we want mocha (describe...it style) or tape

@feross thoughts?

keeping the build simple is great. no babel ftw.

'standard',
'mocha',
'babel-core',
'babel-preset-es2015'
Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot these 😉

Copy link
Contributor Author

@Goldob Goldob Jul 9, 2016

Choose a reason for hiding this comment

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

Oops! Thanks for that!

@@ -1,4 +1,4 @@
language: node_js
node_js:
- 'node'
install: npm install standard
install: npm install standard mocha
Copy link
Member

Choose a reason for hiding this comment

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

I don't prefer mocha because it pollutes the global namespace.

@feross
Copy link
Member

feross commented Jul 28, 2016

@Goldob Great changes! This is good work. Thanks for doing this. I prefer tape to mocha because it's a better citizen. It doesn't leak a bunch of globals and force you to write code that assumes they'll just be there.

@dcposch I don't think we should have any unit tests in WebTorrent Desktop. This app should be mostly glue code. That can be tested best with a few sanity-check integration tests.

If there's a file that feels like it needs unit tests, then it should probably just be it's own npm package with it's own README, unit tests, etc. LocationHistory should definitely be it's own npm package. Probably, torrent-poster and a few others in lib/ as well.

I'm going to pull out location-history into it's own package today.

}
}

LocationHistory.prototype.hasBack = function () {
return this._history.length > 1
return this._back.length
Copy link
Member

Choose a reason for hiding this comment

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

This should return a boolean, so this._history.length > 0 is better.

@feross
Copy link
Member

feross commented Jul 29, 2016

state.location.go({
url: 'home',
onbeforeload: (cb) => {
state.window.title = config.APP_WINDOW_TITLE
Copy link
Member

Choose a reason for hiding this comment

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

this is a good idea! there are actually more places where setTitle() calls can be removed. i'll do that!

@feross
Copy link
Member

feross commented Jul 29, 2016

New PR here: #748

@feross feross closed this Jul 29, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants