[FIX] Handle hash change in url #337

akashdsouza · 2017-09-01T14:31:15Z

#335

cc @MartinMalinda

akashdsouza · 2017-09-01T14:34:57Z

I'm also trying to write a couple of acceptance tests. However, I'm not sure as how to defer the assertions

MartinMalinda · 2017-09-03T16:34:43Z

As long as it will trigger page refresh it's not gonna be testable.

Some ideas:

Make hashToQuery return the new url, instead of doing the redirect directly
Create a shim for this function
Add an initializer, import this function shim there, add an eventListener for hashchange
Lookup the router service and in the hashchange callback, do router.transitionTo(newURL) (it should accept URL afaik)

akashdsouza · 2017-09-07T10:18:06Z

I have made some changes. @MartinMalinda do these changes look ok?

MartinMalinda · 2017-09-07T10:20:17Z

lib/hash-to-query/vendor/hash-to-query/index.js

@@ -0,0 +1,32 @@
+(() => {


lib/hash-to-query/vendor/hash-to-query/index.js that looks a bit strange file path, is that intentional?

I just looked up ember-router-service-polyfill addon for reference. But then again, this is just an in-repo addon. Any suggestions?

I am only wondering about placement of the file lib -> hash-to-query -> vendor -> hash-to-query -> index.js.

MartinMalinda · 2017-09-07T10:21:03Z

app/instance-initializers/hash-to-query-redirect.js

+export function initialize(application) {
+  let redirectToUrl = hashToQuery();
+  if (redirectToUrl) {
+    window.location.href = window.location.origin + redirectToUrl;


I would do this first check already in the vendor file, it will happen sooner.

It will also need typeof FastBoot === undefined check there.

I would do this first check already in the vendor file, it will happen sooner.

I don't understand. Wouldn't we need to have this check before setting location href?

It will also need typeof FastBoot === undefined check there

If this check can be moved to vendor file, can't window check also be placed along with that

Yes I would move it as a whole:

let redirectToUrl = hashToQuery(); if (redirectToUrl) { window.location.href = window.location.origin + redirectToUrl; }

To the vendor file.

If this check can be moved to vendor file, can't window check also be placed along with that

If FastBoot is undefined, window will be defined. No need to double check for window.

MartinMalinda · 2017-09-07T10:24:14Z

tests/unit/initializers/hash-to-query-redirect-test.js

+});
+
+// Replace this with your real tests.
+test('it works', function(assert) {


It would be good to have some input/output tests for the hashToQuery function here. (unless the test coverage for it is elsewhere already)

Simple acceptance test would be cool as well.

Added tests for hashToQuery

We need to verify that legacy links with hashes work (as the currently do today)

MartinMalinda · 2017-09-07T10:30:10Z

lib/hash-to-query/vendor/hash-to-query/index.js

+(() => {
+  /* globals define */
+  function hashToQuery() {
+    var hash = window && window.location && window.location.hash;


This is overly defensive. hashToQuery should be called only when window.location exists.

MartinMalinda · 2017-09-07T20:27:42Z

tests/unit/initializers/hash-to-query-redirect-test.js

+  initialize(this.appInstance);
+
+  let redirectToUrl = hashToQuery('method', '/class');
+  assert.equal(redirectToUrl, '/class?anchor=method&show=inherited,protected,private,deprecated', 'hashToQuery function works');


try these tests with some real data scenarios:

let redirectToUrl = hashToQuery('#method', '/class'); assert.equal(redirectToUrl, '/class?anchor=method&show=inherited,protected,private,deprecated', 'hashToQuery function works'); assert.equal(hashToQuery('#method_pushPayload', '/ember-data/2.14/classes/DS.Store'), '/ember-data/2.14/classes/DS.Store/methods/pushPayload?anchor=pushPayload&show=inherited,protected,private,deprecated'); assert.equal(hashToQuery('#method_pushPayload', '/ember-data/2.14/classes/DS.Store/methods/createRecord'), '/ember-data/2.14/classes/DS.Store/methods/pushPayload?anchor=pushPayload&show=inherited,protected,private,deprecated'); assert.equal(hashToQuery('#method_push', '/ember-data/2.14/classes/DS.Store/methods/createRecord'), '/ember-data/2.14/classes/DS.Store/methods/push?anchor=push&show=inherited,protected,private,deprecated');

Only the second one passes. The others fail because they contain /methods/. Either the hashToQuery function needs to be updated or a new function needs be made that shares some of the logic.

MartinMalinda · 2017-09-07T20:54:16Z

I'm looking into this a little bit more the situation on the initial load of the app and the internal hashchange are a bit different.

Maybe reusing the hashToQuery function is not the best way and just having two different function would be better.

I hacked this together really quick and it works:

import hashToQuery from 'hash-to-query';
import Inflector from 'ember-inflector';

const {inflector} = Inflector;

export function initialize(application) {
  if (typeof FastBoot === 'undefined') {
    window.onhashchange = function() {
      const { hash } = window.location;
      if (hash.includes('#') && hash.includes('_')) {
        const [type, anchor] = hash.replace('#', '').split('_');
        const router = application.lookup('service:router');
        const currentRoute = router.get('currentRouteName');
        const baseRoute = currentRoute.split('.').splice(0, 3).join('.');
        const subRoute = `${inflector.pluralize(type)}.${type}`;
        router.transitionTo(`${baseRoute}.${subRoute}`, {
          queryParams: { anchor }
        });        
      }
    }
  }
}

export default {
  name: 'hash-to-query-redirect',
  initialize
};

MartinMalinda · 2017-09-07T21:09:17Z

We can also update to 2.15 and the polyfill does not need to be installed

MartinMalinda · 2017-09-08T14:46:09Z

@akashdsouza would it be fine for you if I pull your commit and create a new PR with my changes?

akashdsouza · 2017-09-08T14:52:36Z

@MartinMalinda Sure. Go ahead

toddjordan · 2017-09-08T17:08:49Z

lib/hash-to-query/index.js

@@ -8,33 +8,8 @@ module.exports = {
    return true;
  },

-  contentFor (type, config) {


is this really going to work for direct url access, such as say I have a bookmarked link for something with #method_toString?

toddjordan · 2017-09-08T17:10:18Z

lib/hash-to-query/index.js

@@ -8,33 +8,8 @@ module.exports = {
    return true;
  },

-  contentFor (type, config) {


if we are taking out the need for contentFor, then this doesn't need to be an in-repo addon. just make it a util function.

toddjordan · 2017-09-08T17:11:42Z

lib/hash-to-query/vendor/hash-to-query.js

+      }
+    }
+  }
+  if (typeof FastBoot === 'undefined') {


what's this for? Should never enter this if I don't think...

toddjordan · 2017-09-08T18:24:22Z

tried to deploy a heroku preview app for this but got an error running yarn:

toddjordan · 2019-03-28T04:55:43Z

Going to drop support for legacy method/property links, making this fix moot.

Fixing index.md files for 1.10

akashdsouza changed the title ~~WIP:[FIX] Handle hash change in url~~ [FIX] Handle hash change in url Sep 7, 2017

akashdsouza force-pushed the bugfix_redirect_on_hash_change branch from 9f14289 to 51e5513 Compare September 7, 2017 09:12

MartinMalinda reviewed Sep 7, 2017

View reviewed changes

[FIX] Handle hash change in url

8a31915

akashdsouza force-pushed the bugfix_redirect_on_hash_change branch from 6c97a7c to 8a31915 Compare September 7, 2017 16:52

MartinMalinda suggested changes Sep 7, 2017

View reviewed changes

toddjordan reviewed Sep 8, 2017

View reviewed changes

locks temporarily deployed to ember-api-docs-staging-pr-337 September 8, 2017 17:17 Inactive

locks temporarily deployed to ember-api-docs-staging-pr-337 September 8, 2017 17:29 Inactive

toddjordan closed this Mar 28, 2019

Gaurav0 pushed a commit to Gaurav0/ember-api-docs that referenced this pull request Sep 16, 2019

Merge pull request ember-learn#337 from ember-learn/fix-1.10

20c82b9

Fixing index.md files for 1.10

Uh oh!

[FIX] Handle hash change in url #337

[FIX] Handle hash change in url #337

Uh oh!

Conversation

akashdsouza commented Sep 1, 2017

Uh oh!

akashdsouza commented Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MartinMalinda commented Sep 3, 2017

Uh oh!

akashdsouza commented Sep 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinMalinda Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinMalinda Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinMalinda commented Sep 7, 2017

Uh oh!

MartinMalinda commented Sep 7, 2017

Uh oh!

MartinMalinda commented Sep 8, 2017

Uh oh!

akashdsouza commented Sep 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toddjordan commented Sep 8, 2017

Uh oh!

toddjordan commented Mar 28, 2019

Uh oh!

Uh oh!

akashdsouza commented Sep 1, 2017 •

edited

Loading

MartinMalinda Sep 7, 2017 •

edited

Loading

MartinMalinda Sep 7, 2017 •

edited

Loading