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

feat(internal): Support globalThis in modern environments #4628

Merged
merged 2 commits into from
Apr 6, 2020

Conversation

warmrobot
Copy link
Contributor

fix #3561

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests tests with npm test or yarn test)

Copy link
Member

@arxpoetica arxpoetica left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't actually tested, but scanning it looks right.

@Conduitry
Copy link
Member

The code change itself does look good. The test failure looks to be probably an artifact of our test setup, and not an actual bug with the new code.

Favoring window over globalThis when selecting the globals object seems to make them all pass. I haven't yet gotten all the tests to pass when favoring globalThis, and I don't know whether that's going to end up being a worthwhile endeavor.

@warmrobot
Copy link
Contributor Author

Tests passed on my mac, but failed on Parallels Ubuntu 18.04
It seems, that it failed to call scrollTo.

scrollTo$ is not a function

I try to debug test component this way:

import { beforeUpdate } from 'svelte'
	import  { globals } from '/home/parallels/Projects/svelte/internal'
	export let scrollY;

	beforeUpdate(() => {
		console.log('===================', globals.window.scrollTo, globals.scrollY, globals.pageYOffset)
	})

And results were:

=================== [Function] undefined undefined
=================== [Function] undefined undefined

Why globalThis important?
I think it is best approach when working in v8 environment (not Node). We already discussed this with you a few time ago.
Since then we have 2 new projects on Svelte on high load production. We've updated v8 to v8.0. So the last step is to support it in Svelte internals.

@Conduitry
Copy link
Member

The test failure or success was presumably related to the version of Node (and whether it supported globalThis - using newer versions fails because using globalThis rather than window broke tests) and not the operating system. After spending a bit more time on this, I've just pushed a change to make it favor window over globalThis (which shouldn't have any effect in practice) so that the tests pass.

@Conduitry Conduitry merged commit 085897c into sveltejs:master Apr 6, 2020
@warmrobot
Copy link
Contributor Author

I checked it on v8 and it works perfectly. Now we can call Svelte truly 'environment agnostic'. Thank you! )

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.

Make SSR environment-agnostic (don't count on 'global')
3 participants