veliovgroup / flow-router

🚦 Carefully extended flow-router for Meteor
https://packosphere.com/ostrio/flow-router-extra
BSD 3-Clause "New" or "Revised" License
202 stars 29 forks source link

Cannot read property 'replace' of undefined when clicking browser back // production only bug #32

Closed dylanmcgowan closed 6 years ago

dylanmcgowan commented 6 years ago

Hey guys, I think this is the right place to post this, please correct me if I'm wrong. I recently deployed a meteor project to this url

After loading the root url and programmatically routing to another path (with FlowRouter.go), then clicking the browser's back button this error pops up.

Uncaught TypeError: Cannot read property 'replace' of undefined

I deployed the project with the --debug flag and the error occurs in the _.each block with path.replace on line 435..

Router.prototype.initialize = function () {
    function initialize() {
      var options = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : {};

      if (this._initialized) {
        throw new Error('FlowRouter is already initialized');
      }

      var self = this;

      this._updateCallbacks(); // Implementing idempotent routing
      // by overriding page.js`s "show" method.
      // Why?
      // It is impossible to bypass exit triggers,
      // because they execute before the handler and
      // can not know what the next path is, inside exit trigger.
      //
      // we need override both show, replace to make this work
      // since we use redirect when we are talking about withReplaceState                                              

      _.each(['show', 'replace'], function (fnName) {
        var original = self._page[fnName];

        self._page[fnName] = function (path, state, dispatch, push) {
          if (!self.env.reload.get() && self._current.path === path) {
            return;
          }

          `path = path.replace(/\/\/+/g, '/');    // 435`
          original.call(this, path, state, dispatch, push);
        };
      }); // this is very ugly part of pagejs and it does decoding few times
      // in unpredicatable manner. See #168      
      // this is the default behaviour and we need keep it like that
      // we are doing a hack. see .path()

      this._page.base(this._basePath);

      this._page({
        decodeURLComponents: true,
        hashbang: !!options.hashbang
      });

      this._initialized = true;
    }

    return initialize;
  }();

When the back button is clicked, the route path (in the url bar) changes to the correct path, but the corresponding template does not render.

This bug can be reproduced by going to this url, clicking "save me money", then clicking the browser back button.

On the shop insurance template there's an X with a click event including FlowRouter.go('/') Weirdly enough, after clicking that X when the error is thrown, the root url template loads and then the error won't happen again until you refresh the browser.

Note: everything works as intended locally, this only happens in production

Any help would be much appreciated, and if you need any more details please let me know!

Thanks, Dylan

dr-dimitru commented 6 years ago

Hello @dylanmcgowan ,

Please update this issue with:

As I remember this was previously discussed, at the meteor's forum, right? Does that discussion got a conclusion?

dr-dimitru commented 6 years ago

I'm able only to reproduce it at Safari.

dylanmcgowan commented 6 years ago

Hey @dr-dimitru nice seeing you here! Still no resolve over at the Meteor forums so I brought the issue over here. Weird how that only happens in Safari for you, every device & browser I've tested on has the same issue (Chrome, FF, Safari, and their mobile versions). Thanks for your quick response and fix though! I can't update the flow-router-extra package yet, is there some timeline or thing that needs to happen for it to go live so i can test?

Heres a sample of how I render a template in all my views.. I'm using BlazeLayout - is that still the right move?

FlowRouter.route('/', {
  action() {
    BlazeLayout.render('appLayout', { main: 'Landing_page' });
  },
  name: 'home'
});
dr-dimitru commented 6 years ago

I can't update the flow-router-extra package yet, is there some timeline or thing that needs to happen for it to go live so i can test?

Need to fix some other things today, should be published soon. Sorry for the delay.

I'm using BlazeLayout - is that still the right move?

Yes, you can still use the BlazeLayout, but recommended way is build-in .render() method. Take a look on our Templating tutorials

dr-dimitru commented 6 years ago

Hello @dylanmcgowan ,

Published as v3.4.1. Thank you for contribution, hope this will fix your issue.

Feel free to reopen it in case if the issue still persists on your end.

dylanmcgowan commented 6 years ago

Thanks so much for your time @dr-dimitru! Unfortunately the issue is still persisting so we may need to re open this.

I changed my routes to use the render() method instead of BlazeLayout and here's what that code looks like:

import { FlowRouter } from 'meteor/ostrio:flow-router-extra';

import '../../../ui/layouts/app-layout.js';
import '../../../ui/pages/landing/landing-page.js';

FlowRouter.route('/', {
  action() {
    this.render('appLayout', 'Landing_page');
  },
  name: 'home'
});

The error isn't getting thrown to the console any more, but the same buggy behavior is occurring. If there's anything I can do to help let me know. Hope we can resolve this, you've been a massive help thus far!

dr-dimitru commented 6 years ago

@dylanmcgowan

The possible reasons for this behaviour:

  1. You have a route with no defined path (the first argument)
  2. You're using/combining FlowRouter with some other solution for routing
  3. You're using non-FR methods for navigation, you should only use:
dr-dimitru commented 6 years ago

@dylanmcgowan any news on this one?

dylanmcgowan commented 6 years ago

Hey @dr-dimitru, only bad news 😢

1.Each of my routes has a defined path. 2.The only routing package in my app is ostrio:flow-router-extra

  1. I'm using FlowRouter.go('/url-path') in all event maps

I also tried using the {{urlFor}} helper instead of FlowRouter.go() but the same browser history back bug occurs. I don't know what else to do.

dr-dimitru commented 6 years ago

Hello @dylanmcgowan ,

Try to change FlowRouter.go('/url-path') to FlowRouter.go('routeName').

dr-dimitru commented 6 years ago

Hello @dylanmcgowan ,

Any news on this one?

dr-dimitru commented 6 years ago

@dylanmcgowan what do you use for analytics? okgrow:analytics? See https://github.com/meteor/meteor/issues/9419#issuecomment-347274743

dylanmcgowan commented 6 years ago

Wow that looks like the fix! Do you think they'll update their meteor package, or do i need to refactor my app to use the npm one?

dr-dimitru commented 6 years ago

@dylanmcgowan so you're using okgrow:analytics?? If so, yes, you need to switch to NPM package, or wait for an update at Atmosphere.

dylanmcgowan commented 6 years ago

Yes I am using that okgrow:analytics package! Thank goodness I was loosing my mind over here. Thanks so much @dr-dimitru you've been such a massive help :)

dr-dimitru commented 6 years ago

Hey @dylanmcgowan , I'm glad to help :) This is the long one ))

This issue took time to poke around. Thanks to community, Open Source, and @derwaldgeist . Do you ever imagine how this could be solved under close environments. Damn! I love Open Source (IMHO it should be always be written starting with capital letters)!

Feel free to close it once you will successfully update package, and your issue will be gone.

derwaldgeist commented 6 years ago

Yeah, I absolutely love OpenSource. In the corporate world, you would wait for such an update for months. Thanks!