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

not compatible with SSR #7

Closed
fatlinesofcode opened this issue Jul 17, 2019 · 11 comments
Closed

not compatible with SSR #7

fatlinesofcode opened this issue Jul 17, 2019 · 11 comments

Comments

@fatlinesofcode
Copy link

doesnt work when you import into a SSR app.

you should add

if(typeof window !== 'undefined')

before kicking off the polyfill

@wessberg
Copy link
Owner

wessberg commented Jul 17, 2019

Hi there. Thanks for submitting this issue. I do not agree that this polyfill doesn't work with SSR in a general sense, but if you mean that you're trying to evaluate the polyfill from Node.js then yes, it does refer to browser globals and works with these. I can add a compatibility check that ensures that these globals are in fact present, such as document and window

@fatlinesofcode
Copy link
Author

i was attempting to use it with Next.js.
The plugin crashes the SSR, its fine in the CSR

@wessberg
Copy link
Owner

wessberg commented Jul 17, 2019

Yes, my point is that anything that can be serialized into a string and sent to the client is SSR-compatible, same goes for this polyfill. Some SSR-solutions such as the one from Next.js require partial evaluation ahead of time to precompute among other things the initial state of your components and so it needs to evaluate things like the imports of the files that hold your components in a Node environment and yes, this polyfill is not written in isomorphic JavaScript, but instead relies on the presence of browser globals.

What you can do (you may already do this) is to use JSDOM to shim a browser environment in Node to patch the globals that this polyfill and potentially other of your present or future dependencies may depend on that references globals that is present only in browser environments. And I will look into adding a guard.

@fatlinesofcode
Copy link
Author

Next, dont seem to encourage the use of the jsdom. vercel/next.js#6669

@wessberg
Copy link
Owner

Not using JSDOM or in any other way shimming a browser environment means that you as a developer has to make sure that all of your code, including the code you depend on (such as this polyfill) can execute in Node. But I would assume that any Web application will eventually need to operate on the DOM or use a member of the Node interface.

You'll have this issue in lots of other polyfills as well, so I would recommend using something like polyfill.app to conditionally apply the polyfill only for the browsers that actually need it. As an added benefit, it will keep your bundle size small and your loading times fast 👍

@fatlinesofcode
Copy link
Author

fatlinesofcode commented Jul 17, 2019

Not using JSDOM or in any other way shimming a browser environment means that you as a developer has to make sure that all of your code, including the code you depend on (such as this polyfill) can execute in Node. But I would assume that any Web application will eventually need to operate on the DOM or use a member of the Node interface.

Thats correct. We absolutely want to avoid executing dom functionality in the SSR. We rely on the lifecycle methods componentDidMount() in React and mounted() in Vue. Or env var process.browser

You'll have this issue in lots of other polyfills as well,

We don't. Most 3rd party packages we use include a check for 'window'

@wessberg
Copy link
Owner

My recommendation is for you to conditionally import the polyfill based on whether or not it is already supported and whether the environment is right, SSR or not. I already suggested polyfill.app, but you could also just do:

if (typeof window !== "undefined" && !("scrollBehavior" in document.documentElement.style)) {
	await import("scroll-behavior-polyfill");
}

As I write in the README, adding a blocking import that forces every browser to parse and evaluate the polyfill even if it is already supported or in your case not environmentally compatible is discouraged.

You can also leave your import as it is and I will ensure that the polyfill is compatible with isomorphic JavaScript use cases such as yours when I have some time.

@beeirl
Copy link

beeirl commented Jul 17, 2019

@fatlinesofcode Is it possible to share a code example of how you accomplished to get it working with Next.js? I am struggeling a bit. Would be really great.

@wessberg
Copy link
Owner

@Duux, have you tried conditionally applying the polyfill with a dynamic import expression as in my example above?

@beeirl
Copy link

beeirl commented Jul 17, 2019

@wessberg Yeah, I did the following:

./client/polyfills.js

import dynamic from 'next/dynamic'

if (typeof window !== 'undefined' && !('scrollBehavior' in document.documentElement.style)) {
   dynamic(() => import('scroll-behavior-polyfill'))
}

next.config.js

webpack: (config, options)  => {
      config.entry = async () => {
         const entries = await originalEntry()

         if (entries['main.js'] && !entries['main.js'].includes('./client/polyfills.js')) {
            entries['main.js'].unshift('./client/polyfills.js')
         }

         return entries
      }
}

_document.js

...
<html style={{ scrollBehavior: 'smooth' }}>
...
</html>
...

@wessberg
Copy link
Owner

I've made the polyfill compatible with execution in non-browser environments. Please report back to me if this doesn't solve the issue. It has been published as part of v2.0.9.

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

No branches or pull requests

3 participants