electerious / basicScroll

Standalone parallax scrolling for mobile and desktop with CSS variables.
https://basicscroll.electerious.com
MIT License
3.63k stars 148 forks source link

Replace deepcopy with ImmutableJS' fromJS().toJS() #2

Closed btoo closed 7 years ago

btoo commented 7 years ago

hi electerious, you got an awesome parallax library going here, but i came across a bug that ive actually encountered before on a different project that was also using the deepcopy npm module (i doubt the bug has anything to do with your code).

the bug looked like this: Unhandled Rejection (TypeError): Cannot assign to read only property 'replace' of object '[object Location]' and i've narrowed down the source of it to the deepcopy line in validate(). i have no idea why this is happening (im using this in a react project, if that means anything), but ive i found that replacing it with ImmutableJS' fromJS( {object to be copied} ).toJS(), which creates a deep clone, did the trick

electerious commented 7 years ago

Can you show how you are using basicScroll / the call throwing the error? Would be interesting to know.

Switching to a different library than deepcopy isn't a problem. I just wonder if ImmutableJS is a bit overpowered for this case. It might increases the size of the bundle, too.

btoo commented 7 years ago

at a high level, this is what's happening in a react component i was using to implement your parallax demo:

import React, { Component } from 'react'
import {findDOMNode} from 'react-dom'

const basicScroll = require('basicscroll')

export default class BasicScroll extends Component {

  componentDidMount(){
    [...findDOMNode(this).querySelectorAll('.scene')]
      .map(elem => basicScroll.create({
        elem,
        from  : 0,
        to    : 519,
        props : {
          '--translateY': {
            from   : '0',
            to     : `${ 10 * elem.dataset.modifier }px`,
            direct : true
          }
        }
      }).start())
  }

  render() { return (
    <div>
      <img className="scene" data-modifier="30" src="https://parallaxbilder-qmogbogaoz.now.sh/p0.png" />
      <img className="scene" data-modifier="18" src="https://parallaxbilder-qmogbogaoz.now.sh/p1.png" />
      <img className="scene" data-modifier="12" src="https://parallaxbilder-qmogbogaoz.now.sh/p2.png" />
      <img className="scene" data-modifier="8" src="https://parallaxbilder-qmogbogaoz.now.sh/p3.png" />
      <img className="scene" data-modifier="6" src="https://parallaxbilder-qmogbogaoz.now.sh/p4.png" />
      <img className="scene" data-modifier="0" src="https://parallaxbilder-qmogbogaoz.now.sh/p6.png" />
    </div>
  )}
}
electerious commented 7 years ago

Interesting. The code looks fine as far as I can tell.

Could you try it with loadash.clonedeep?

btoo commented 7 years ago

yes it does!

electerious commented 7 years ago

Perfect! Please create a new PR with loadash.clonedeep and I'm happy to merge your changes :)