BookStackApp / BookStack

A platform to create documentation/wiki content built with PHP & Laravel
https://www.bookstackapp.com/
MIT License
15.37k stars 1.92k forks source link

Built JavaScript is polluting global scope #5232

Closed ssddanbrown closed 1 month ago

ssddanbrown commented 1 month ago

We're building via esbuild in ESM format (https://esbuild.github.io/api/#format-esm) For which they state:

Do not forget type="module" as this will break your code in subtle and confusing ways (omitting type="module" means that all top-level variables will end up in the global scope, which will then collide with top-level variables that have the same name in other JavaScript files).

So we need to ensure scripts are fetched as modules. Browser compatibility looks okay: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#browser_compatibility Will just need some general testing to ensure nothing messes up, specifically since the script will be deferred. Need to check with all elements loaded after the script.

CiberNin commented 1 month ago

If I override layouts/base.blade.php to change line 77 to <script type="module" src="{{ versioned_asset('dist/app.js') }}" nonce="{{ $cspNonce }}"></script>, the pages show begins functioning with my 3rd party library perfectly.

However the edit page was not so fortunate. Note that with this error I have removed the import of my third party library & the only change from tock is the aforementioned base.blade.php edit.

app.js?version=v24.05.4:3 Failed to create component TypeError: Cannot read properties of undefined (reading 'JustifyLeft')
    at uo (app.js?version=v24.05.4:7:19630)
    at app.js?version=v24.05.4:9:396
    at YO.setup (app.js?version=v24.05.4:22:1450)
    at new YO (tinymce.min.js?version=v24.05.4:4:417082)
    at tinymce.min.js?version=v24.05.4:4:426996
    at Array.<anonymous> (tinymce.min.js?version=v24.05.4:4:427101)
    at Re (tinymce.min.js?version=v24.05.4:4:5335)
    at Object.s (tinymce.min.js?version=v24.05.4:4:426839)
    at Ta.executeHandlers (tinymce.min.js?version=v24.05.4:4:55285)
    at i (tinymce.min.js?version=v24.05.4:4:53123) wysiwyg-editor 

Which presumably means we need to change how tinymce is imported with app.js being a module now. Unfortunately that's not a thing I can do with the theme system.

ssddanbrown commented 1 month ago

Resolved via 34ade5018186b0acc01205dd1ed3c6b1043f835e.

@CiberNin because of some bad references in the prior code you'd need to patch those too (which can't be done via theme system and is not really advised) otherwise you can wait until the next feature release for these changes to be included.