nearform / react-animation

Animation components and styles for React projects
https://nearform.github.io/react-animation/
Apache License 2.0
286 stars 20 forks source link

Does not work in combination with Gatsby #16

Open Kilian opened 5 years ago

Kilian commented 5 years ago

In the code you do a pretty good job of checking if window exists, but it seems your build tool does this to the beginning of the dist file:

!(function(e, t) {
  'object' == typeof exports && 'object' == typeof module
    ? (module.exports = t(require('react')))
    : 'function' == typeof define && define.amd
    ? define('ReactAnimation', ['React'], t)
    : 'object' == typeof exports
    ? (exports.ReactAnimation = t(require('react')))
    : (e.ReactAnimation = t(e.React));
})(window, function(e) {

That window on the last line makes a gatsby build fail, since window is undefined in that context. This means it can't get built, and that you can't use react-animation in Gatsby.

donovanh commented 5 years ago

Thanks for the message! My apologies for being slow to respond. I'll see what can be done to fix it here 👍

juliangoacher commented 5 years ago

Replacing the window reference with this by adding globalObject: 'this' to the output section of webpack.dist.config.js should fix this.

https://webpack.js.org/configuration/output/#outputglobalobject

mcollina commented 5 years ago

@juliangoacher is there anything we can do in this library to support it?

juliangoacher commented 5 years ago

@mcollina I'm putting together a PR with the suggested change now, fix should be available very soon

juliangoacher commented 5 years ago

@mcollina Fix for this problem is now available in https://www.npmjs.com/package/react-animation/v/1.1.1

fbaiodias commented 5 years ago

Hey @juliangoacher! I've just tried using react-animation@1.1.1 in a Next.js project and also got ReferenceError: window is not defined at r (./node_modules/react-animation/dist/react-animation.js:1:4184) 😟 r is r=function(){return window&&document&&document.all&&!window.atob}, which seems to come from here: https://github.com/webpack-contrib/style-loader/blob/master/src/addStyles.js#L23. There's a related issue in that repo that might be useful: https://github.com/webpack-contrib/style-loader/issues/272

juliangoacher commented 5 years ago

Hi @fbaiodias - thanks for pinpointing the exact problem in your report. It seems the only real option here is to try and exclude the webpack style loader from the distribution build. I don't have any next.js environment I can test against; would you mind testing the attached and see if it improves the situation? (Strip the .txt from the end - had to add this otherwise github won't allow the file to be attached).

react-animation.js.txt

fbaiodias commented 5 years ago

Hey @juliangoacher! Just tested it with the Default animation and it seems to work now 🎉 Thanks for the fast response 🙌

juliangoacher commented 5 years ago

@fbaiodias Ok great - but I'm still not 100% sure if this is the way to fix it, some functionality may be dependent on the style loader, need to review the code in a bit more detail.

Can you tell me - shouldn't this be a common problem on next.js? I'm sure there's plenty of react components that use style loader.

fbaiodias commented 5 years ago

@juliangoacher I'm not sure to be honest, I’ve never ran into this issue before 😕

juliangoacher commented 5 years ago

@fbaiodias This lib uses style-loader to import animation keyframes into the different components, so the lib still works with style-loader excluded but animation quality is affected. I've been looking at options like switching to an isomorphic style loader https://www.npmjs.com/package/isomorphic-style-loader but that then puts an onus on the library user to add boilerplate to make the entire thing work. Still looking at a few other options, I'll post an update here hopefully later today.

juliangoacher commented 5 years ago

@fbaiodias Would you be able to try the attached in your env and see if it runs ok? I've rewritten the code to use styled components, which removes the need for the webpack style loader. Don't know if you need to do some next.js specific configuration to make it run, here https://www.styled-components.com/docs/advanced#server-side-rendering might have some info on that.

react-animation.js.txt

fbaiodias commented 5 years ago

@juliangoacher It's not working and I'm getting this error now: Warning: Prop `className` did not match. Server: "animate-on-change animate-on-change- sc-bxivhb gmunBs" Client: "animate-on-change animate-on-change- sc-bdVaJa fMcbSQ" Could you try using styled-components as a peerDependency instead of bundling it? I've had issues in the past with multiple instances of styled-components: https://www.styled-components.com/docs/faqs#why-am-i-getting-a-warning-about-several-instances-of-module-on-the-page

juliangoacher commented 5 years ago

@fbaiodias styled-components is only a dev dependency so shouldn't be included in the bundle. Is it possible that this https://github.com/styled-components/babel-plugin-styled-components/issues/78 is related?

juliangoacher commented 5 years ago

@fbaiodias Or possibly this is related: https://stackoverflow.com/a/54272027/8085849 - everything I'm reading suggests it's a config issue with next.js

fbaiodias commented 5 years ago

@juliangoacher Strange, somehow this string ended up in the file you uploaded: An error occurred. See https://github.com/styled-components/styled-components/blob/master/packages/styled-components/src/utils/errors.md#. It seems to come from here: https://github.com/styled-components/styled-components/blob/master/packages/styled-components/src/utils/error.js#L49

Edit: just checked those links, I've had styled-components and babel-plugin-styled-components setup like that and working well in this project for a while now, inclusively with dependencies that have styled-components as peerDependency 🤔

juliangoacher commented 5 years ago

@fbaiodias I'm a bit out of ideas at the moment. You have styled-components as a peer dependency, but also as a dev dependency. styled-components was a dev dependency of react-animate before my changes (it was used in the example page, but nowhere else) so i don't think the packaging of the bundle is the issue. I see that your package.json specifies version ^4.1.1 of styled-components; mine specifies ^4.3.2; I've attached a build done using version 4.1.1, in case this is a version mismatch problem and you want to test it. I'm going to read back over the links above, in case it could be something else mentioned in them.

react-animation.js.txt

juliangoacher commented 5 years ago

@fbaiodias I've decided to take a completely different approach, and instead to create a separate distribution for SSR usage. To do this, I've created a new project https://github.com/juliangoacher/react-animation-isomorphic; the project is a thin wrapper around react-animation and separate webpack config that uses https://github.com/kriasoft/isomorphic-style-loader for style loading, so the hope is that this should provide a fully next.js compatible solution.

If you want to try it out then you will need to do the following:

The last step has been the complicating factor in this. I didn't want to introduce isomorphic-style-loader into the base package as it would force all users to add a context provider, but it seems a reasonable requirement for SSR usage.

juliangoacher commented 5 years ago

I've tried testing this change on next.js myself, and long story short it doesn't work; seems that configuring next.js to use isomorphic-style-loader is anything but straightforward (see https://github.com/timmywil/next.js/tree/e06a09c4d795e94e011e77128b8c47725165020e/examples/with-isomorphic-style-loader) and it looks to me like its use precludes the use of other style loaders (e.g. @zeit/next-css). Not sure at this point whether the best thing to do is to just include sample code in the project showing the basics of how to use the lib with isometric-style-loader, and leave it to the user to figure out the specifics of how to integrate into their platform of choice.

luchoster commented 5 years ago

I had the same issue happened to me with mapbox-gl, I just ran into this again with react-animations, since you're referencing Gatsby, then the way to fix this, is by adding webpack configuration as mentioned here: https://www.gatsbyjs.org/docs/debugging-html-builds/#fixing-third-party-modules

Here's my gatsby.node.js:

exports.onCreateWebpackConfig = ({ stage, loaders, actions }) => {
  if (stage === 'build-html') {
    actions.setWebpackConfig({
      module: {
        rules: [
          {
            test: /mapbox-gl/,
            use: loaders.null(),
          },
          {
            test: /react-animation/,
            use: loaders.null(),
          },
        ],
      },
    })
  }
}

This fixes the issue on your Gatsby build.

stovv commented 4 years ago

I found workaround for NextJS. Works for me, maybe someone will help

render(){
        if (typeof window === "undefined"){
            return (<></>);
        }
        const {AnimateOnChange} = require('react-animation');
        return (
            <AnimateOnChange>
               {/*some items*/}
            </AnimateOnChange>
        );

    }