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

Shim being loaded as a script is an issue #67

Closed
akrigline opened this issue Aug 1, 2021 · 3 comments
Closed

Shim being loaded as a script is an issue #67

akrigline opened this issue Aug 1, 2021 · 3 comments
Labels
bug Something isn't working released

Comments

@akrigline
Copy link
Contributor

I'm seeing Not allowed to re-assign the global instance of libWrapper while both this module and LibWrapper is installed. I confirmed with ruipin and this is because the libwrapper shim is expected to be used as an imported esmodule.

I'm guessing this is from the JE shim, which will be obsolete soon anyway?

@ruipin
Copy link

ruipin commented Aug 1, 2021

The problem is that this file is loaded as scripts, rather than esmodules. As such, it is running in global scope, and the libWrapper symbol it creates will be visible by every other module, even though it is not the full library.

While the shim will check for libWrapper.is_fallback before using it, many modules that do not use the shim might not, and as such this might actually cause modules to believe the library is installed when it is not.

In addition, if more than one module does this, it is unknown which module will run later and thus which shim will "win out". As such, you cannot predict which implementation the shim has. It is possible another module would run after yours with an older version of the shim that does not support something you rely on, or that is missing bugfixes, breaking your module.

These reasons are why the libWrapper documentation states in bold:

Please do not make your custom shim available in the global scope.

It is also why the default shim exports libWrapper, rather than assigning it to global scope. You removed all this from your shim.

I would ask that you change this whenever possible so that this file is imported, rather than loading it in global scope as a script. It is better for your module, and also for other modules.

@eXaminator
Copy link
Owner

Thanks for letting me know, looking at the discord there seems to have been a bit of confusion around this issue in this regard.

Well, I didn't want to update this package to use the new (soon to be released) sheet registrar lib until I actually had a bug or feature to release with it... I guess I have a bug fix on my hands now ;)

I'll fix this as soon as I can, hopefully (but not quite guaranteed) sometime tonight (CEST).

@eXaminator eXaminator added the bug Something isn't working label Aug 2, 2021
eXaminator added a commit that referenced this issue Aug 2, 2021
This should fix problems with the libWrapper library

Fixes #67
github-actions bot pushed a commit that referenced this issue Aug 2, 2021
## [2.3.5](2.3.4...2.3.5) (2021-08-02)

### Bug Fixes

* exchange shim with dependency to _document-sheet-registrar ([b99db12](b99db12)), closes [#67](#67)
@github-actions
Copy link

github-actions bot commented Aug 2, 2021

🎉 This issue has been resolved in version 2.3.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

No branches or pull requests

3 participants