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

Memory leak, sort of. Events get called multiple times #8

Closed
jacwright opened this issue May 26, 2019 · 3 comments
Closed

Memory leak, sort of. Events get called multiple times #8

jacwright opened this issue May 26, 2019 · 3 comments

Comments

@jacwright
Copy link
Contributor

jacwright commented May 26, 2019

At the beginning of listen you wrap the history's pushState and replaceState so they dispatch a function when called. The problem is, if you call unlisten and listen again, or have multiple instances of navaid in the page, those wrapped functions get wrapped over and over and dispatch multiple events each page change.

https://github.com/lukeed/navaid/blob/master/src/index.js#L43

@frederikhors
Copy link

Same problem here. @lukeed this seems serious.

@lukeed
Copy link
Owner

lukeed commented Jun 3, 2019

RE: memory leak for unlisten() -> listen() is definitely a problem and will be fixed regardless.


There's not really a good way to handle multiple routers within the same page... currently.
But what do you guys think about this for handling multiple routers in a page?

var main = (
  navaid()
    .on('/', () => console.log('Home'))
    .on('/about', () => console.log('About'))
);

var items = (
  navaid()
    .on('/items/', () => console.log('Items Home'))
    .on('/items/:title', x => console.log('Item: ', x.title))
);

main.use(items);
main.listen();

I already have this built, and it seems to be the only sane way to juggle multiple routers on a page. It's currently 867B which is +35B.

As for actual routing, it will look at the main routes first and then the sub-routers' routes in the order that they were defined (via use() hookups).

@lukeed lukeed closed this as completed in 0f5b5bb Jun 3, 2019
@lukeed
Copy link
Owner

lukeed commented Jun 3, 2019

Fixed the memory leak. We can discuss multiple routers separately: #10

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

No branches or pull requests

3 participants