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

Rename 'Blacklight' inside of core.js as 'Core' #3355

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

jrochkind
Copy link
Member

@jrochkind jrochkind commented Oct 10, 2024

When bundled into one file such as the 'blacklight.esm.js' being produced, it was resulting in a conflict with top-level Blacklight module, and resulting in error 'Uncaught TypeError: Blacklight.activate is not a function'

While Javascript modules are normally isolated namespaces and you wouldn't expect a conflict, something about how bundlers end up bundling them together means that when the onload callback is actually called (at runtime, after the bundle has been bundled), and tries to call Blacklight, it ends up refering to a different top-level 'Blacklight' module -- one that doens't have an 'activate' property, instead has 'Core', 'Modal', etc -- the default exports. Instead of capturing the local one under closure as desired.

Blacklight.listeners().forEach(function(listener) {
document.addEventListener(listener, function() {
Blacklight.activate()
})
})

The current produced 'blacklight.esm.js' file is not actually useable (also in BL8). This makes it useable again.

The problem is present in the produced ./app/assets/javascripts/blacklight/blacklight.esm.js file which this gem already produces (using rollup), and I think I also reproduced it with esbuild.

Renaming to const name to Core to avoid a conflict seems to resolve the problem -- and follows the pattern in all of the other module files.

It should not cause a problem or be backwards incompatible for anyone else using ESM modules (including importmap-rails!), precisely because the names inside the module are supposed to be isolated inside the module, the exports are still the same and still used the same. In my manual tests it seemed to be good, and should also be demonstrated not causing a problem by tests being green here hopefully!

I plan to backport this to BL8 if/once merged!

When bundled into one file such as the 'blacklight.esm.js' this file produces, it was resulting in a conflict with top-level Blacklight module, and resulting in error 'Uncaught TypeError: Blacklight.activate is not a function'
@jrochkind jrochkind changed the title Rename 'Blacklight' inside of core.js as 'BlacklightCore' Rename 'Blacklight' inside of core.js as 'Core' Oct 10, 2024
@tpendragon tpendragon merged commit 2e95685 into main Oct 10, 2024
10 checks passed
@tpendragon tpendragon deleted the avoid_js_module_name_clash branch October 10, 2024 15:31
@jrochkind
Copy link
Member Author

Preserving some more context for the reocrd:

It took me a long time to debug because it is not doing what we think the tools should do, I don’t entirely get it.
I managed to solve my problem by making this file use variable names parallel to the other files, so that seemed like a good solution to me.

If you take a checkout from before this PR was merged, and buidl the blacklight.esm.js file, and import it somewhere — I believe you will be able to reproduce with the error I reported there.

Let me try to actually look at source for that pre-merge blacklight.esm.js to see if I can see something that makes it more clear….

This line:
https://gist.github.com/jrochkind/476e9a94eefc9ff816c22789f2c49298#file-blacklight-esm-js-L377

The Blacklight ref inside that callback is supposed to refer to this object:
https://gist.github.com/jrochkind/476e9a94eefc9ff816c22789f2c49298#file-blacklight-esm-js-L339

And it seems like it would.

But when it is actually executed, (when the addEventListner calls it, not sure if it was in response to a page:load or a turbo event or what, prob a turbo event?)….

The Blacklight reference there somehow ends up refering to this object:
https://gist.github.com/jrochkind/476e9a94eefc9ff816c22789f2c49298#file-blacklight-esm-js-L392-L401

And getting an error “no such property activate”, since it doesn’t have one. I confirmed this is definitely what’s going on with lots of debugging.

Why is it referring to that? Maybe because that winds up stored in window.Blacklight (not sure?) and the rules of JS resolution somehow get there?

I am honestly not sure (spent most of the day yesterday on this), but the fix fixed it and seemed appropriate.

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.

2 participants