Update plugin to support multiple plugins in the same project #41
Conversation
|
That's a very useful feature 👍🏻 |
|
Hi @mic345 Thanks for the PR! I'd prefer to see this broken into several PRs, e.g., one for TS2794, one for the script loading issue, and one for your new functionality (I've added some more comments to the pr regarding these). Can you also add a description of the use case that you are solving for? |
src/web.ts
Outdated
| @@ -289,15 +289,15 @@ export class FirebaseAnalyticsWeb extends WebPlugin | |||
| document.getElementById(scripts[0]) && | |||
| document.getElementById(scripts[1]) | |||
| ) { | |||
| return resolve(); | |||
| return resolve( null ); | |||
There was a problem hiding this comment.
Instead of passing a value where none is expected, can you modify the promise to declare a type, e.g.:
return new Promise<void>(async (resolve, _reject) => {
| @@ -273,26 +273,8 @@ export class FirebaseAnalyticsWeb extends WebPlugin | |||
| /** | |||
| * Check for existing loaded script and load new scripts | |||
| */ | |||
| private loadScripts() { | |||
There was a problem hiding this comment.
When you split this out into a separate PR, can you add some details about what is going wrong and what you did to fix?
There was a problem hiding this comment.
can you add some details about what is going wrong and what you did to fix?
If I remember correctly, we did this in order to create a reusable code between firebase-analytics and firebase-remote-config for their common needs (loading the scripts, etc.).
In an ideal world someone would create a firebase-common project, move all the common code there and would use that project both firebase-analytics, firebase-remote-config and anywhere else firebase needs to load scripts (likely everywhere). Over time more common code would move there.
When you split this out into a separate PR,
I apologize for the single PR. We issue those fixes in the same PR as we needed to proceed with our work and the original project was discarded. Different PRs would have never been merged on time.
You are rightfully asking for splitting, but I'm afraid I wont have time for it anytime soon, as I'm already over loaded with work 😬. At this point I can suggest either to create a branch on the original project, merge the whole PR to it and test/refactor as much as needed, and only than merge to the master, or to cherry pick the commits from the PR. If it was me, I'd go for the first option, as the entire PR is already tested and used on our project.
I hope you find our code helpful.
In short this pull request includes: