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

Error when several instances with different root configurations #1055

Closed
Jonnyprof opened this issue Feb 25, 2022 · 8 comments
Closed

Error when several instances with different root configurations #1055

Jonnyprof opened this issue Feb 25, 2022 · 8 comments
Labels

Comments

@Jonnyprof
Copy link

It happens to me in version 6.1.5, but I guess it happens too in the lastest version, because isPlatformBrowser function (the main problem) has the same code. I'll try to explain it as best as possible.

When you register several instances, they can reuse IntersectionObserver with findEntry function.

const foundedEntry = this.findEntry(config.root, config.hash);

The error occurs because config.hash is always false because isPlatformBrowser returns always false.

Let's go step by step:
1- Hash is calculated in InViewportConfig contructor calling a static function.

this._hash = InViewportConfig.hash({

2- Static hash function calls toBase64 static function.

private static hash(input: object): string {
return toBase64(InViewportConfig.stringify(input));
}

3- toBase64 depends on isPlatformBrowser.

export function toBase64(input: string): string {
return !isPlatformBrowser()
? toBase64Node(input)
: toBase64Browser(input);
}

4- isPlatformBrowser always return false, because this === window is always false.

export function isPlatformBrowser() {
try {
return typeof window !== 'undefined' && this === window;
} catch (e) {

5- As isPlatformBrowser is false we call toBase64Node. This function returns always false because of typeof global !== 'undefined'

export function toBase64Node(input: string): string {
try {
return typeof global !== 'undefined' && global.Buffer.from(input).toString('base64');
} catch (e) {

6- So this._hash is always false

this._hash = InViewportConfig.hash({

If we have 2 instances, the first one has a rootMargin = '0px 0px 0px 0px'; and the second one rootMargin = '100px 0px 0px 0px';. When register the instances in the service, both uses same IntersectionObserver because both have same hash (false) and same root.

const foundedEntry = this.findEntry(config.root, config.hash);

and the second one not works with the correct margin, so don't work as expected.

@k3nsei
Copy link
Owner

k3nsei commented Feb 25, 2022

@Jonnyprof could you tell more about your environment? Because I don't see a way how isPlatformBrowser would return false in any browser? this in function which isn't an instance and isn't bound function should always return Window instance.

And can you prepare some example on stackblitz by forking this project https://stackblitz.com/edit/ng-in-viewport-example?embed=1&file=src/main.ts

@Jonnyprof
Copy link
Author

Sorry, I didn't notice your reply.
I don't understand your question, sorry for my english. It happens with angular dev-server (ng serve) or in a web server with production build.
I've created a fork of your stackblitz where you can see (debugging or adding a logPoint) that your function isPlatformBrowser is false. You can see it here https://stackblitz.com/edit/ng-in-viewport-example-6cpco8.
I've attached a screenshot too.
imatge

I'm logging config.hash console.log('hash:', config.hash, 'rootMargin:', config.rootMargin); and it returns always false in a webserver environment but not in stackblitz because there global is defined. global is undefined normally in a browser environment. I've attached a screenshot.
imatge

You can download the stackblitz code and run it on a webserver and hash will be always false.
I've done more tests in a fork of your repository (https://github.com/Jonnyprof/ng-in-viewport/tree/hash-false) on branch hash-false and hash is always false (console.log too).

I hope this help.

@k3nsei
Copy link
Owner

k3nsei commented Mar 11, 2022

I will switch to btoa as now node.js lts have that function in its global scope.

@k3nsei k3nsei added the bug label Jan 29, 2023
@k3nsei
Copy link
Owner

k3nsei commented Jan 29, 2023

@all-contributors please add @Jonnyprof for bug

@allcontributors
Copy link
Contributor

@k3nsei

I've put up a pull request to add @Jonnyprof! 🎉

@k3nsei
Copy link
Owner

k3nsei commented Jan 30, 2023

Should be fixed in v15.0.0

@k3nsei k3nsei closed this as completed Jan 30, 2023
@Jonnyprof
Copy link
Author

Great, thanks! Tell me if you need some testing help.

@k3nsei
Copy link
Owner

k3nsei commented Jan 30, 2023

@Jonnyprof You can install it and test if it works for you. As it was complete rewrite of library, it's now have 100% coverage. But unit tests are not real world scenarios. So it would be beneficial to see if it works in real app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants