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

fix: added reportError polyfil #2192

Merged
merged 2 commits into from
Dec 8, 2023
Merged

Conversation

olehrakwix
Copy link
Collaborator

Added reportError polyfill to allow applications and are using native reportError to execute feature setup.

@@ -1,3 +1,5 @@
import "./polyfills/report-error-polyfill.js";
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for it in web workers?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, I verified that this entrypoint is run for any type of environment, including webworker

Comment on lines 3 to 7
/**
* hope this is single polyfill we need for now
* in case if more will be added, we need to create some
* better mechanism to handle polyfills
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

/**
 * This polyfill is not essential for the engine itself. But applications built
 * on the engine may require this polyfill to correctly report initialization
 * errors in older browsers.
 * Ideally, the engine should offer a mechanism allowing applications to
 * initialize necessary polyfills before the evaluation of feature files begins.
 * But so far we only need this single polyfill.
 */

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks, updated

@olehrakwix olehrakwix merged commit dfeded1 into master Dec 8, 2023
6 checks passed
@olehrakwix olehrakwix deleted the olehr/fix-report-error-polyfill branch December 8, 2023 11:55
@AviVahl
Copy link
Contributor

AviVahl commented Dec 15, 2024

I'm removing this, as this stops the engine-core package from having no side effects.

We need to find a better way to inject polyfills needed by applications.
It should not come as a hack within engine-core, which also caused this method to be put in Node environments.

As a replacement to this change, I'm changing engine-core's reportError to default to globalThis.reportError, with the current implementation used as a fallback. This can be imported and used.

AviVahl added a commit that referenced this pull request Dec 15, 2024
reverts #2192 to remove only sideEffect in engine-core package
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

Successfully merging this pull request may close these issues.

3 participants