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

Configure lazysizes with function argument instead of window #647

Closed
nimi opened this issue Apr 29, 2019 · 15 comments
Closed

Configure lazysizes with function argument instead of window #647

nimi opened this issue Apr 29, 2019 · 15 comments

Comments

@nimi
Copy link

nimi commented Apr 29, 2019

Hi @aFarkas ,

Thanks for the library! We really like this library, but we're struggling to use it in a way that works for us. We don't want to leak lazySizes lazySizesConfig on the window object. Is there any way that the core of the library could be exposed in such a way that would allow us to use a plain JS object for configuration.

This would involve a couple of changes:

Update this function to allow for an optional third argument for configuration

function l(window, document, lazysizesOptions) {
 // ...
 lazySizesConfig = window.lazySizesConfig || window.lazysizesConfig || lazysizesOptions || {};

 if (!lazysizesOptions) {
   window.lazySizesConfig = lazySizesConfig;
 }
 // ...
}

Expose a build of lazysizes-core.js in a way that can be consumed without putting lazysizes onto the window object. So, instead of

import 'lazysizes';

You could do the following

import initLazysizes from 'lazysizes/core';
// ...some other setup code
initLazysizes(window, document, {
  lazyClass: 'my-custom-lazyclass'
})

Thanks again!

@aFarkas
Copy link
Owner

aFarkas commented Apr 29, 2019

@nimi

Is it also ok for you to just remove the following lines:

if (!lazysizesOptions) {
   window.lazySizesConfig = lazySizesConfig;
 }

You can then do this:

import lazySizes from 'lazysizes';

Object.assign(lazySizes.cfg, {
    lazyClass: 'my-custom-lazyclass',
});

@nimi
Copy link
Author

nimi commented Apr 29, 2019

This approach is fine, but

import lazySizes from 'lazysizes'

is going to cause window.lazySizesConfig to be set.

This line

window.lazySizesConfig = lazySizesConfig;

is the thing we don't want. We want an option for this to be configurable/preventable. One way to do that is to change the initialization pattern via object, open to other ideas or suggestions.

@aFarkas
Copy link
Owner

aFarkas commented Apr 29, 2019

Yes, this is what I would delete.

@nimi
Copy link
Author

nimi commented Apr 29, 2019

Yep, that would be awesome. Thanks @aFarkas !

@nimi
Copy link
Author

nimi commented Apr 29, 2019

I like that changes so far, but I do see one issue here @aFarkas that could be a problem your suggested configuration setting pattern. See below for a simplified example (using es2015+ for brevity):

// Somewhere else prior to initializing lazysizes
window.lazysizesConfig = { shouldInit: true }
// Later on we load lazysizes
const lazysizes = (function(window) {
  let cfg = window.lazysizesConfig || {}

  setTimeout(() => {
    if (!cfg.shouldInit) {
      console.log("No init")
      return
    }
    init()
  })

  function init() {)
    console.log(cfg)
  }

  return {
    cfg
  }
})(window)

Object,assign(lazysizes.cfg, {
  shouldInit: false
})

// logs "No init"
console.log(window.lazysizesConfig)
// logs { shouldInit: false }

It seems Object.assign is going to mutate window.lazysizesConfig

@aFarkas
Copy link
Owner

aFarkas commented Apr 29, 2019

I'm now a little bit irritated. As I understood you earlier. You don't want to use the global config option at all. Now you are showing me some code where you are using it.

Can you please explain your hole use case.

Please keep in mind that you don't have to set init to false prior to importing the script. You can do this also after importing it.

As soon as you define the global object it is defined and lazySizes doesn't leak it only references it. In fact now you are leaking it.

@nimi
Copy link
Author

nimi commented Apr 29, 2019

Hi @aFarkas ,

Sorry for the confusion. We don't want to leak global variable setting at all by using this library if possible. It does everything we want it to do, and would like have it work as it's designed without having to worry that we're going to be changing or assigning values to window.lazysizesConfig. We run our code in an environment where a user can configure and add their own custom code.

A quick solution might be to copy values from a window.lazysizesConfig object to a plain object

So instead of

lazySizesConfig = window.lazySizesConfig || window.lazysizesConfig || {};
function assign() {
 // some code to assign and return a fresh object
}
lazySizesConfig = assign(window.lazySizesConfig) || assign(window.lazysizesConfig) || {};

Let me know if this makes sense. I should have clarified that this code might be run by someone else (out of our control)

// Somewhere else prior to initializing lazysizes
window.lazysizesConfig = { shouldInit: true }

@aFarkas
Copy link
Owner

aFarkas commented Apr 29, 2019

First of all I can not really do this. As soon as I destroy the reference if a user decides to create one. The user is not able to change the config on the fly during a later point. Which is a massive API change and I can not do it.

On the other hand I still don't get the underlying issue. What is the problem for you exactly.

So a user want to control the lazysizes that you are importing by adding.

window.lazysizesConfig = { shouldInit: true }

Then you import lazysizes and change the configuration. Why the heck should the object that was created show an old config and not the current config? What problem does it create that lazysizes shows the right/actual/current configuration and not an old outdated one?

@nimi
Copy link
Author

nimi commented Apr 29, 2019

Thanks for the reply @aFarkas .

The problem that we're trying to solve is that we want to configure lazysizes in a way that does not necessitate checking or mutating a global configuration variable. We might have to interact with code that also uses the lazysizes library and it is using the globally defined variable (as it stands that is the only way). We might want to defer initialization or our customers might be doing things on their end (with an instance of lazysizes) that we cannot predict. We want to avoid impacting our code and their code.

If you need to rely on keeping a reference to a window configuration when it's defined, I can understand that you don't want to break the API in that way. I think my original suggestion of allowing configuration via function argument would avoid this problem and solve our problem as well.

90% - 95% lazysizes works great without having to worry about this problem. But, we, unfortunately, have some users that have added this lazysizes intentionally or unknowingly and have their own custom configuration defined on the window object. We don't want to override or affect their configuration

@aFarkas
Copy link
Owner

aFarkas commented Apr 29, 2019

Ok I start to understand. I have the feeling that you are / this is somehow related to the following issue #643 . Your suggested code is indeed working great for this as long as you don't use one of the official plugins (because they are importing lazysizes and changing that would itself be a quite big API change).

My idea is that you can break the public config yourself.

Here is my other suggestion:

You could write a module like this:

// module: break-lazysizes.js
// store lazysizes config, delete config and restore
let config = window.lazySizesConfig;

if ('lazySizesConfig' in window) {
	delete window.lazySizesConfig;
}

export default () => {
	if ('lazySizesConfig' in window) {
		if (config) {
			window.lazySizesConfig = config;
		} else {
			delete window.lazySizesConfig;
		}
	}
};

Then you do the following:

import restoreLazySizesConfig from './break-lazysizes';
import lazySizes from 'lazysizes;

lazySizes.cfg.lazyClass = 'my-foo';
restoreLazySizesConfig();

I know it is not that nice but you are able to use all plugins and you don't even have to wait for a new version. This should be already possible. What do you think?

@nimi
Copy link
Author

nimi commented Apr 29, 2019

Yeah, we'd like a cleaner way of doing this, but this looks like it should work. We'll give this a try. Thanks for your time @aFarkas

@nimi
Copy link
Author

nimi commented Apr 30, 2019

Closing this issue, since we have a now have workaround.

@nimi nimi closed this as completed Apr 30, 2019
@thasmo
Copy link

thasmo commented May 13, 2019

In the changelog for 5.0.0 it is mentioned Do not leak global lazySizesConfig anymore fixes #647 - is this something which has a code/API change in 5.0.0?

@aFarkas
Copy link
Owner

aFarkas commented May 13, 2019

Yes and no. You still can configure lazySizes through the window.lazySizesConfig options object if you declare it before lazySizes is imported. But if you haven't created it yourself lazySizes won't leak it.

@thasmo
Copy link

thasmo commented May 13, 2019

@aFarkas, thanks for the explanation.

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 a pull request may close this issue.

3 participants