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

Should clean the fireworks when ngOnDestroy #90

Merged
merged 2 commits into from
Mar 8, 2024

Conversation

Eric-Guo
Copy link
Contributor

@Eric-Guo Eric-Guo commented Jan 23, 2024

I really want to avoid any but the upstream code not export FireworksInstance

/**
 * @param idOrOptions - the id used for displaying the animation, or the animation configuration if an id is not necessary
 * @param sourceOptions - the animation configuration if an id is provided
 * @returns the loaded instance
 */
export async function fireworks(
    idOrOptions: string | RecursivePartial<IFireworkOptions>,
    sourceOptions?: RecursivePartial<IFireworkOptions>,
): Promise<FireworksInstance | undefined> {

Copy link

sweep-nightly bot commented Jan 23, 2024

Apply Sweep Rules to your PR?

  • Apply: There should not be large sections of commented out code.
  • Apply: All docstrings and comments should be up to date.

Copy link
Contributor

sweep-ai bot commented Jan 23, 2024

Apply Sweep Rules to your PR?

  • Apply: There should not be large sections of commented out code.
  • Apply: All docstrings and comments should be up to date.

This is an automated message generated by Sweep AI.

Copy link
Contributor

@matteobruni matteobruni left a comment

Choose a reason for hiding this comment

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

Please check the review comments, I think the promise should be handled in the constructor instead of in the ngDestroy

@Eric-Guo
Copy link
Contributor Author

Eric-Guo commented Feb 4, 2024

FireworksInstance exported in new version and I using it now.

@matteobruni
Copy link
Contributor

The build is failing, can you please fix it? I cannot merge until it fails

@Eric-Guo
Copy link
Contributor Author

Eric-Guo commented Mar 7, 2024

@matteobruni done and really appreciate your work!

@matteobruni
Copy link
Contributor

Still failing

@Eric-Guo
Copy link
Contributor Author

Eric-Guo commented Mar 7, 2024

Interesting, FireworksInstance exported in already published package, any idea why this error happen?

@matteobruni
Copy link
Contributor

matteobruni commented Mar 7, 2024

I've checked the files of this PR, you are changing only the component, but the dependencies must be updated as well, since that export came in a newer version of @tsparticles/fireworks.

Please build the project locally before submitting commits.

@matteobruni
Copy link
Contributor

Still failing, please build the project locally before submitting other commits.

pnpm install
pnpm run build

in the root folder.

@Eric-Guo
Copy link
Contributor Author

Eric-Guo commented Mar 8, 2024

Should work, thank you for your patience

@matteobruni
Copy link
Contributor

Everything is fine now, thanks!

@matteobruni matteobruni merged commit 690f177 into tsparticles:main Mar 8, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants