collective / collective.lazysizes

Integration of lazysizes, a lightweight lazy loader, into Plone.
https://pypi.org/project/collective.lazysizes
6 stars 2 forks source link

upgrade to 4.1.2, do not include UMD version #68

Closed thet closed 6 years ago

thet commented 6 years ago

Fixes: #67

thet commented 6 years ago

no un/redifinition of define and require but only including the non-UMD version.

hvelarde commented 6 years ago

@thet let's see what @rodfersou thinks on this before merging; do you need a release after that?

thet commented 6 years ago

@hvelarde a release would be awesome. Meanwhile I fork the repo and check the built files into the fork.

thet commented 6 years ago

looks like the built bundle still contains the AMD module definition part. (took me some time to built due to a missing libpng-dev dependency on my system) investigating... meanwhile WIP

hvelarde commented 6 years ago

this could be the issue: https://github.com/collective/collective.lazysizes/blob/4.1.1.1/webpack/webpack.config.js#L20

thet commented 6 years ago

fixed

hvelarde commented 6 years ago

@thet do you mind to explain briefly to me what's the difference among these options?

rodfersou commented 6 years ago

@thet could you please give more information on the error we are facing here? Why a new umd library in the site give problems? This should be the default of new JS libraries.

rodfersou commented 6 years ago

@thet can you guide me on how to make the same environment to research what is going on in this issue?

thet commented 6 years ago

@rodfersou it's the same error with the same cause like described here: https://docs.plone.org/adapt-and-extend/theming/resourceregistry.html#the-mismatched-anonymous-define-error

and the same reason why Plone undefines/redefines require and define for all scripts, which are not managed and compiled via RequireJS. This is done here: https://github.com/plone/Products.CMFPlone/blob/3fb2e9c4ce6772ce05623cf3b85e18a76d13dae2/Products/CMFPlone/resources/browser/scripts.pt

For this to be usable in current RequireJS enabled Plone 5 the scripts must not include a module definition wrapper or undefine require or define like above.

I did not see the error pop up in current Chrome or Firefox - probably because the script was loaded at the very end due to it's async property?

thet commented 6 years ago

I have a fairly complex site, but basically it should be enough to install lazysizes in Plone 5.1 and open it with Firefox 36.

hvelarde commented 6 years ago

@thet the async attribute has been around for many years and is in fact supported by the browsers you mentioned: https://www.caniuse.com/#feat=script-async

my guess is the syntax is not supported in those browsers: can you modify the template on the egg to look like this async="async" and test again, please?

thet commented 6 years ago

I'll try. But still, in Plone 5 you won't ship any resourceregistry and requirejs unmanaged JS with a module definition wrapper.

hvelarde commented 6 years ago

@rodfersou you can read about that in https://requirejs.org/docs/errors.html#mismatch

@thet I understand, but that doesn't explain why it works in newer browser versions.

thet commented 6 years ago

tnx for testing this out @hvelarde

I could reproduce the problem of lazysizes with module definition wrapper with a recent Firefox (63.0b7 Developer Edition) when moving the viewlet below the other JavaScripts. It occurs with and without the async property.

When lazysizes with module definition wrapper is loaded before any other JavaScript recent FF versions do not complain.

hvelarde commented 6 years ago

awesome! I see locally that lazysizes is in fact the first script loaded in my Plone 5.1 site; how did you ended loading it after the others? do you know if there's a way to programmatically guarantee that one viewlet is loaded before others? I'm only aware the viewlets.xml file.

we need to understand better the issue before finding a fix.

hvelarde commented 6 years ago

guys, I think I understand now the issue and seems that it has already being addressed in the past:

seems we have two options:

rodfersou commented 6 years ago

is it possible to make a test with this configuration: https://github.com/plonegovbr/brasil.gov.tiles/blob/master/webpack/webpack.config.js#L14

thet commented 6 years ago

A named AMD module might be a solution. Although I think it makes not much sense, especially in the context of Plone - but maybe I'm wrong....

Regarding how I moved the viewlet slot - I'm using plone.app.blocks based tiles and site layouts. It's easy to move the viewlet-tile whereever I want with that.

hvelarde commented 6 years ago

IIRC, AMD, CommonJS and UMD are just ways to sandbox and encapsulate JavaScript code; for me it makes a lot of sense in Plone if we're going to keep stuff separated.

rodfersou commented 6 years ago

@thet could you please try to use the option:

libraryExport: 'default',

To see if this resolves the problem? I remember that when we move from webpack2 to webpack3 the library was exported as default.libraryname instead of libraryname. Maybe this fix the problem.

Take a look at this configuration https://github.com/plonegovbr/brasil.gov.tiles/blob/master/webpack/webpack.config.js#L14

hvelarde commented 6 years ago

@thet any news on this? were you able to test the fix?