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

Update page to 1.11.5 #75

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Update page to 1.11.5 #75

wants to merge 2 commits into from

Conversation

jankapunkt
Copy link

@jankapunkt jankapunkt commented Feb 23, 2020

NOTE: this is a work in progress PR attempting to fix #74 , since there are multiple issues to address. Not all issues may have been detected yet and regression needs to be tested as well.

Currently detected issues:

  • _basePath is chained infinitely

This isse is fixed by always transforming the path to a /name pattern. I am not 100% if the page package now won't accept other path variations as base anymore or has a bug or something.

  • _basePath is stripped. after initialize is called

This is fixed by setting the dispatch option in the page(options) call to false. This fixes the Client - Router - base path tests. However, there is no regression-test added yet.

  • exit triggers are fired twice

We first need to find out, why the exit trigger is fired twice now, while before once

@@ -633,7 +637,7 @@ Tinytest.add('Client - Router - base path - url generation', (test) => {

function setBasePath(path) {
FlowRouter._initialized = false;
FlowRouter._basePath = path;
FlowRouter.basePath(path);
Copy link
Member

Choose a reason for hiding this comment

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

@jankapunkt Wouldn’t it be major changes?
Maybe we can change _basePath to setter instead of function?

dr-dimitru added a commit that referenced this pull request Jun 8, 2022
@dr-dimitru
Copy link
Member

@jankapunkt do you want to try update page on the latest release? I have couple of things fixed

@jankapunkt
Copy link
Author

@dr-dimitru does it make sense to continue this one, since Meteor3 upgrade is breaking anyway?

@jankapunkt
Copy link
Author

@dr-dimitru also, it looks like page is abandoned and people look for a fork.

@dr-dimitru
Copy link
Member

@jankapunkt I'll jump on full rewrite sometimes soon, dropping all dependencies. We can close this one for now.

Latest release works with meteor@3 as expected, right?

@jankapunkt
Copy link
Author

@dr-dimitru it does not support Blaze 3, which is why I commented on #113 to include it

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

Successfully merging this pull request may close these issues.

Update page NPM dependency [help-wanted]
2 participants