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

Framework needs to inform container when app js fails to load #108

Closed
ilinkuo opened this Issue Jun 7, 2013 · 4 comments

Comments

Projects
None yet
3 participants
@ilinkuo

ilinkuo commented Jun 7, 2013

In https://github.com/OpenF2/F2/blob/master/sdk/src/container.js, lines 237-239

    error:function(jqxhr, settings, exception) {
        F2.log(['Failed to load script (' + e +')', exception.toString()]);
    }

the Framework needs to inform the Container that there's an error rather than just logging. This might be either by a function call to something provided by the Container upon initialization, or via an event broadcast. The Container needs to know this in order to do any cleanup or retry.

While you're at it, you might also consider doing so for css load failures as well, though I'm less concerned with that.

The case of manifest load failure is already covered because I can actually rather easily override F2.removeApp().

@markhealey markhealey referenced this issue Sep 16, 2013

Merged

F2 version 1.3.1 #138

6 of 6 tasks complete
@ilinkuo

This comment has been minimized.

Show comment
Hide comment
@ilinkuo

ilinkuo Sep 17, 2013

  • In the unit tests, there's a remark that "Seems to take longer than the other tests for some reason". This might be due to failure mode. The unit tests only cover failure mode for 404 file not found, it seems. You might try other failure modes such as a 500 error, nonexistent domain, https file not found, and invalid protocol to make sure all these errors get thrown.
  • Adding all these tests could really slow down your Travis CI build, though.

ilinkuo commented Sep 17, 2013

  • In the unit tests, there's a remark that "Seems to take longer than the other tests for some reason". This might be due to failure mode. The unit tests only cover failure mode for 404 file not found, it seems. You might try other failure modes such as a 500 error, nonexistent domain, https file not found, and invalid protocol to make sure all these errors get thrown.
  • Adding all these tests could really slow down your Travis CI build, though.
@markhealey

This comment has been minimized.

Show comment
Hide comment
@markhealey

markhealey Oct 2, 2013

Member

@mtangolics do you want to take a look at the failure modes mentioned by @ilinkuo? We don't want to slow down the build significantly, so let's review your findings.

Thanks.

Member

markhealey commented Oct 2, 2013

@mtangolics do you want to take a look at the failure modes mentioned by @ilinkuo? We don't want to slow down the build significantly, so let's review your findings.

Thanks.

@mtangolics

This comment has been minimized.

Show comment
Hide comment
@mtangolics

mtangolics Oct 3, 2013

Contributor

@ilinkuo I later determined that the delay was due to the scriptErrorTimeout config option defaulting to a value of several seconds, so I reduced it to 100ms in the unit tests.

Regarding the additional failure modes: I had originally considered adding these in, but I ran some tests and determined that the XHR error callback catches any scenario where the script fails to load regardless of the error code returned, so I don't think it will be necessary to add these at this time.

Contributor

mtangolics commented Oct 3, 2013

@ilinkuo I later determined that the delay was due to the scriptErrorTimeout config option defaulting to a value of several seconds, so I reduced it to 100ms in the unit tests.

Regarding the additional failure modes: I had originally considered adding these in, but I ran some tests and determined that the XHR error callback catches any scenario where the script fails to load regardless of the error code returned, so I don't think it will be necessary to add these at this time.

@markhealey

This comment has been minimized.

Show comment
Hide comment
@markhealey

markhealey Oct 3, 2013

Member

Thanks @mtangolics. Closing since #140 has been merged.

Member

markhealey commented Oct 3, 2013

Thanks @mtangolics. Closing since #140 has been merged.

@markhealey markhealey closed this Oct 3, 2013

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