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

Possible to send actual objects via callback.redirect? #129

Open
sw-clough opened this issue Aug 23, 2019 · 10 comments
Open

Possible to send actual objects via callback.redirect? #129

sw-clough opened this issue Aug 23, 2019 · 10 comments

Comments

@sw-clough
Copy link

My app is using push state routing. In my resolve functions, I am doing some checks before returning a successful callback. For example, you visit book/50, and I do a check to ensure that book 50 exists, the user has permission, etc. If there is a problem, I am doing a callback.redirect() to an error page, and sending an error object in the parameters. However, the object doesn't make it intact to the error state's resolve function, as it gets turned into a parameter on the query line. Is there a way to pass the actual error object to the error state's resolve function?

@TehShrike
Copy link
Owner

ASR tries to follow a fundamental principle of browser routing: that users should be able to send anyone else a link to your current URL, or hard-refresh the page, and see essentially what they saw before.

Generally speaking, if you want to preserve certain aspects of an error before redirecting to an error page, you need to decide on a way to serialize them to the URL, so that the principles of routing can continue to hold.

But your choice is a bit more involved, first you have to choose – do you want users to refresh the page in order to try to reload whatever page they were trying to view before, or should the users hit the back button to try to go to the non-error page they were at before?

Or said another way, what would you want to happen if someone who was sent to that error page copied the URL and sent it to someone else to open?

@TehShrike
Copy link
Owner

@sw-clough
Copy link
Author

Sorry, hadn't had time to review your response until now.

Regarding the choice you presented, I hadn't thought about it. If you have an error page with parameters in the query string, then people could effectively send around joke error pages with custom URL strings. That doesn't seem ideal. So maybe leaving the URL the same, but showing an error page as the content, is the way to go.

I get the fact that you should be able to copy-paste the URL and see the same page, and passing around objects shortcuts that. However, it wouldn't be hard to check for existence of the object in the activate function, and obtain if required. Of course, I could attach some sort of cache object to the data field as I create the state, and maybe that is answer, but I could see this being a function of this package too.

I've come across another scenario that could benefit from passing an object, in regards to child states. Say I have the states app.book with route book\:id and app.book.sellers with route \sellers, and both templates need to query for the book with the ID, then my code currently does that request twice, once in each resolve/template function. If the parent could pass the object to the child, then that should be more efficient.

@sw-clough
Copy link
Author

sw-clough commented Aug 27, 2019

So after thinking about it more, I like the idea of showing the error at the problem URL, without a redirect. So now I have implemented a helper function to render the Error template instead of the "good" template, like the following. It's not the ideal solution, but if you are reluctant to introduce a method to achieve this in a different way, I guess it will have to do.

function RenderUnlessError(template) {
    return function(options) {
        if (
            options.props.hasOwnProperty("error") &&
            options.props.error !== null
        ) {
            options.props.context = options.props.error
            return new ErrorPage(options)
        }
        return new template(options)
    }
}

stateRouter.addState({
    name: "app.areas",
    route: "/areas",
    template: RenderUnlessError(Areas),
    data: { services: services },
    resolve: (data, params, callback) => {
        data.services.area.getTopLevelAreas().then(
            (areas) => {
                callback(null, { areas: areas, ...data })
            },
            (error) => {
                let error_context = {
                    title: "Problem Loading Data",
                    error: error,
                    state: "app.areas",
                    route: "/areas",
                    parameters: { ...params }
                }
                callback(null, { error: error_context })
            }
        )
    }
})

@TehShrike
Copy link
Owner

That does seem like the ideal solution for now.

I would be open to a PR that added a setErrorTemplate method that took a template as its only argument.

The behavior I'm imagining would, if a stateChangeError or stateError event was fired, render that template, passing in the error.

lastCompletelyLoadedState and lastStateStartedActivating could probably be passed in too.

The template would have to be rendered on the root element that was passed in when the router was created.

@saibotsivad
Copy link
Collaborator

That sounds like opt-in behaviour, so I guess it wouldn't be too bad, but it sounds a bit more application specific than I'd like in an abstract router, especially since (it looks like) you can just watch for those error events and do that yourself.

What I do in a several apps is watch for error events, and then based on the error I may do some business logic, go to a different route, or pass the error to some other component:

asr.on('stateChangeError', error => {
    if (someBusinessLogic(error)) {
        doSomething()
    } else if (someOtherLogic(error) {
        asr.go(someRoute, someOptions)
    } else {
        errorComponent.open(error)
    }
})

It doesn't seem very likely that you'd want all errors handled exactly by rendering an error template--you'd want to watch for e.g. API auth errors (user logged out) or other things.

Maybe I'm misunderstanding? As I understand it, I don't think I'd ever use that convenience method in any app that I'd build, so in that sense it feels like clutter.

@saibotsivad
Copy link
Collaborator

One limitation I do note, is that stateError and stateChangeError don't currently pass along lastCompletelyLoadedState or lastStateStartedActivating, and I could definitely see that being useful information to have.

Maybe changing from stateError(err) to something like stateError(err, opts) would give the needed information, without adding more complexity?

@TehShrike
Copy link
Owner

@sw-clough is there anything that you would miss if you went with that kind of solution?

asr.on('stateChangeError', error => {
	new ErrorPage({
		target,
		data: {
			error
		}
	})
})

This pattern should probably at least be mentioned in the readme

@sw-clough
Copy link
Author

sw-clough commented Aug 27, 2019

What I do in a several apps is watch for error events, and then based on the error I may do some business logic, go to a different route, or pass the error to some other component:

Going to a different route is what I was doing, but that requires passing parameters in the URL, which I am trying to avoid. It also means the error doesn't get shown at the current URL.

Regarding some other component, I'm not sure what this implies (maybe it is what TehStrike mentions in next comment?).

@sw-clough is there anything that you would miss if you went with that kind of solution?
This pattern should probably at least be mentioned in the readme

I don't see how this would work, due to the following issues;

  1. Where has target come from?
  2. As this is no longer a pre-configured state, I believe it won't get all the default data/props that other states would.
  3. If this error is on stateChangeError, has the URL changed or not?
  4. I am using svelte-state-renderer. My example function RenderUnlessError is being called at this location: https://github.com/TehShrike/svelte-state-renderer/blob/f3ad3fbb049c723bc3aadc8b3cebbe9d534a7495/index.js#L19, which is called by line 26 or 28. Then, the code from lines 43-48 runs. I believe all of this code would need to be executed?

@TehShrike
Copy link
Owner

@sw-clough

Where has target come from?

You would use whatever root element you passed in when you instantiated the router.

As this is no longer a pre-configured state, I believe it won't get all the default data/props that other states would.

The default parameters you passed to the renderer?

You could use the renderer to instantiate the error page:

asr.on('stateChangeError', error => {
	renderer.render({
		element: rootElement,
		template: ErrorPage,
		content: { error }
	})
})

If this error is on stateChangeError, has the URL changed or not?

With ASR, state changes are driven by URL changes. The url changes to /app/a, and then the app.a state begins loading. If an error happens during the state loading, the url would still be at /app/a.

My example function RenderUnlessError is being called at this location ... I believe all of this code would need to be executed?

I don't quite follow – the stateChangeError examples above are a suggested alternative to the RenderUnlessError method.

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