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

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

Closed
dylanmcgowan opened this issue Nov 8, 2017 · 17 comments

Comments

@dylanmcgowan
Copy link

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
Copy link
Member

Hello @dylanmcgowan ,

Please update this issue with:

  • Version of flow-router-extra you're experiencing this issue
  • Version of Meteor you're experiencing this issue
  • Browser name and its version (Chrome, Firefox, Safari, etc.)?
  • Platform name and its version (Win, Mac, Linux)?
  • How do you render templates? (Code sample please)

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

@dr-dimitru
Copy link
Member

I'm able only to reproduce it at Safari.

@dylanmcgowan
Copy link
Author

dylanmcgowan commented Nov 10, 2017

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'
});
  • The package version is ostrio:flow-router-extra@3.4.0
  • Meteor version is 1.6 (Was happening on 1.5 as well)
  • My machine is a Mac running High Sierra @ 10.13

@dr-dimitru
Copy link
Member

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 added a commit that referenced this issue Nov 10, 2017
 - Dependencies update
 - Compatibility with `meteor@1.6`
 - Fix #32 , thanks to @dylanmcgowan
 - Implement #30 , add support for all hooks into `Group` definition.
Thanks to @THETCR
@dr-dimitru
Copy link
Member

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
Copy link
Author

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
Copy link
Member

dr-dimitru commented Nov 11, 2017

@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
Copy link
Member

@dylanmcgowan any news on this one?

@dylanmcgowan
Copy link
Author

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
3. 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
Copy link
Member

Hello @dylanmcgowan ,

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

@dr-dimitru
Copy link
Member

Hello @dylanmcgowan ,

Any news on this one?

@dr-dimitru
Copy link
Member

@dylanmcgowan what do you use for analytics? okgrow:analytics? See meteor/meteor#9419 (comment)

@dylanmcgowan
Copy link
Author

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
Copy link
Member

@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
Copy link
Author

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
Copy link
Member

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants