Skip to content
This repository has been archived by the owner on Sep 7, 2022. It is now read-only.

Firebase not loaded exception #103

Merged
merged 6 commits into from
Jun 8, 2017

Conversation

Janamou
Copy link
Contributor

@Janamou Janamou commented Apr 17, 2017

Can you please take a look @kevmoo? Issue: #97.

Thx

Copy link
Contributor

@kevmoo kevmoo left a comment

Choose a reason for hiding this comment

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

Would be nice to have a test for this. Just create a test without HTML – and verify the throw.

@@ -0,0 +1,10 @@
<!DOCTYPE html>
Copy link
Contributor

Choose a reason for hiding this comment

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

QQ: did you try this test w/out an HTML file at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Janamou this is the only tweak I'd like a reply to...then I'm happy to land this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevmoo Sorry, I panicked because of my bug with tests in Webstorm :/
BUT, I am solving the thing that the test will not pass on other platforms than Dartium. Because I am testing there the instance of FirebaseJsNotLoadedException via throwsA(new isInstanceOf<fb.FirebaseJsNotLoadedException>()) but compiled to JS is ReferenceError and the test will not pass.

Is there any smarter solution than test only "throws"?

Copy link
Contributor

Choose a reason for hiding this comment

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

See 529f330

Seems that we don't handle this consistently between Dartium and dart2js – if you're bored you could file an issue on the SDK for this...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevmoo yes, that's it, thanks! Will do an issue.

Copy link
Contributor

@kevmoo kevmoo left a comment

Choose a reason for hiding this comment

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

One last nit: update pubspec to 3.0.3 and add an entry into the changelog?

Thanks so much!

* master:
  Prepare to release v3.0.2
  support the latest version of pkg/func
  Use latest Travis-CI dart tasks
  Latest release update
@Janamou
Copy link
Contributor Author

Janamou commented Apr 21, 2017

@kevmoo done, thx!

@kevmoo
Copy link
Contributor

kevmoo commented Apr 21, 2017

How do you want to handle #104 ?

We could call this 3.0.3-dev in the pubsec then rebase?

@Janamou
Copy link
Contributor Author

Janamou commented Apr 21, 2017

@kevmoo mmt, working on #104 now :)

@Janamou
Copy link
Contributor Author

Janamou commented Apr 27, 2017

@kevmoo and this? :-)

@kevmoo
Copy link
Contributor

kevmoo commented Apr 27, 2017

Ah! I thought this was rolled into the other one.

Wanna rebase?

@Janamou
Copy link
Contributor Author

Janamou commented Apr 27, 2017

@kevmoo yes, can you rebase it please? Thx

@kevmoo kevmoo merged commit 4601b49 into googlearchive:master Jun 8, 2017
@Janamou Janamou deleted the firebase-not-loaded branch September 24, 2017 08:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants