wadackel/sweet-scroll

container is not set when code is compressed with webpack

nicolechung opened this issue ยท 28 comments

There is a line in getContainer:

if (!container && !isDomContentLoaded) {

The problem with this line seems to happen when sweet scroll is minified and loads before the DOM is ready.

Like, I think the test for the container needs to be somehow changed so that it fires before the DOM loads, not sure the best way to do that??

Basically if the container is still null but if the DOM has loaded it skips to this line and then silently fails:

callback.call(this, container); // container is null

I think it should be:

if (!container || !isDomContentLoaded) {

If you want to see by running some logs, here is a quick and dirty test to see what I mean:

{
      key: "getContainer",
      value: function getContainer(selector, callback) {
        var _this3 = this;

        var _options = this.options;
        var verticalScroll = _options.verticalScroll;
        var horizontalScroll = _options.horizontalScroll;

        var container = null;

        if (verticalScroll) {
          container = scrollableFind(selector, "y");
        }

        if (horizontalScroll) {
          container = scrollableFind(selector, "x");
        }
        console.log({
          container: container,
          isDomContentLoaded: isDomContentLoaded
        });
        if (!container && !isDomContentLoaded) {
          console.log('no dom content yet');
          (function () {
            var isCompleted = false;

            addEvent(doc, DOM_CONTENT_LOADED, function () {
              isCompleted = true;
              _this3.getContainer(selector, callback);
            });

            // Fallback for DOMContentLoaded
            addEvent(win, "load", function () {
              if (!isCompleted) {
                _this3.getContainer(selector, callback);
              }
            });
          })();
        } else {
          console.log('dom is ready now, container is not always');
          console.log({
            container: container
          })

          callback.call(this, container);
        }
      }

Also, as a sidenote, you add an Event (addEvent) when the dom hasn't loaded yet, but this doesn't seem to get removed once you reach the callback.call line?

Hi @nicolechung. Thanks you for issues.
I will confirm that afterward ๐Ÿ˜ƒ

It was confirmed by Chrome (52.0.2743.116 (64-bit)) but was not able to reproduce ... ๐Ÿ˜ข

In the following code have been removed the container, but do you have any reason? (Removed when compiled with webpack?)

if (horizontalScroll) {
  container = scrollableFind(selector, "x");
}

You've got a good point. DOMContentLoaded and load event is likely to need to be canceled ๐Ÿ‘


Can you send me a sample code including webpack.config.js?

Here is the webpack config:

base

var path = require('path')

module.exports = {
  entry: {
    app: './src/main.js'
  },
  output: {
    path: path.resolve(__dirname, '../dist/static'),
    publicPath: 'static/',
    filename: '[name].js'
  },
  resolve: {
    extensions: ['', '.js', '.vue'],
    fallback: [path.join(__dirname, '../node_modules')],
    alias: {
      'src': path.resolve(__dirname, '../src'),
      'config': path.join(__dirname, '../config', process.env.NODE_ENV)
    }
  },
  resolveLoader: {
    fallback: [path.join(__dirname, '../node_modules')]
  },
  module: {
    preLoaders: [
      {
        test: /\.vue$/,
        loader: 'eslint',
        exclude: /node_modules/
      },
      {
        test: /\.js$/,
        loader: 'eslint',
        exclude: /node_modules/
      }
    ],
    loaders: [
      {
        test: /\.vue$/,
        loader: 'vue'
      },
      {
        test: /\.js$/,
        loader: 'babel',
        exclude: /node_modules/
      },
      {
        test: /\.json$/,
        loader: 'json'
      },
      {
        test: /\.html$/,
        loader: 'vue-html'
      },
      {
        test: /\.(png|jpg|gif|svg)$/,
        loader: 'url',
        query: {
          limit: 10000,
          name: '[name].[ext]?[hash:7]'
        }
      },
      {
        test: /\.css$/,
        loader: 'style-loader!css-loader'
      },
      {
        test: /\.scss$/,
        loaders: ["style", "css", "sass"]
      }
    ]
  },
  eslint: {
    formatter: require('eslint-friendly-formatter')
  }
}

prod

var webpack = require('webpack')
var config = require('./webpack.base.conf')
var cssLoaders = require('./css-loaders')
var ExtractTextPlugin = require('extract-text-webpack-plugin')
var HtmlWebpackPlugin = require('html-webpack-plugin')

config.bail = true

// naming output files with hashes for better caching.
// dist/index.html will be auto-generated with correct URLs.
config.output.filename = '[name].[chunkhash].js'
config.output.chunkFilename = '[id].[chunkhash].js'

// whether to generate source map for production files.
// disabling this can speed up the build.
var SOURCE_MAP = true

config.devtool = SOURCE_MAP ? '#source-map' : false

config.vue = config.vue || {}
config.vue.loaders = config.vue.loaders || {}
cssLoaders({
  sourceMap: SOURCE_MAP,
  extract: true
}).forEach(function (loader) {
  config.vue.loaders[loader.key] = loader.value
})

config.plugins = (config.plugins || []).concat([
  // http://vuejs.github.io/vue-loader/workflow/production.html
  new webpack.DefinePlugin({
    'process.env': {
      NODE_ENV: '"build"'
    }
  }),
  new webpack.optimize.UglifyJsPlugin({
    compress: {
      warnings: false
    },
    mangle: {
      except: ['SweetScroll']
    }
  }),
  new webpack.optimize.DedupePlugin(),
  new webpack.optimize.OccurenceOrderPlugin(),
  // extract css into its own file
  new ExtractTextPlugin('[name].[contenthash].css'),
  // generate dist index.html with correct asset hash for caching.
  // you can customize output by editing /index.html
  // see https://github.com/ampedandwired/html-webpack-plugin
  new HtmlWebpackPlugin({
    filename: '../index.html',
    template: 'index.html',
    inject: true,
    minify: {
      removeComments: true,
      collapseWhitespace: true,
      removeAttributeQuotes: true
      // more options:
      // https://github.com/kangax/html-minifier#options-quick-reference
    }
  })
])

module.exports = config

this is the result code after webpack (looks like horizontalScroll is not removed for me?)

{
          key: "getContainer",
          value: function getContainer(selector, callback) {
            var _this3 = this;

            var _options = this.options;
            var verticalScroll = _options.verticalScroll;
            var horizontalScroll = _options.horizontalScroll;

            var container = null;

            if (verticalScroll) {
              container = scrollableFind(selector, "y");
            }

            if (!container && horizontalScroll) {
              container = scrollableFind(selector, "x");
            }

                // note: if container is null HERE but Dom is loaded, it goes to the callback!
            if (!container && !isDomContentLoaded) {
              (function () {
                var isCompleted = false;

                addEvent(doc, DOM_CONTENT_LOADED, function () {
                  isCompleted = true;
                  _this3.getContainer(selector, callback);
                });

                // Fallback for DOMContentLoaded
                addEvent(win, "load", function () {
                  if (!isCompleted) {
                    _this3.getContainer(selector, callback);
                  }
                });
              })();
            } else {
              callback.call(this, container);
            }
          }

Like, without being to recreate the DOM issue (you need to have a big webpack with lots of JS to recreate) you can see that it's possible for the container to be null AND the dom to be loaded. If so, because the conditional uses && instead of || it's possible for the callback to be called with null container (if that makes sense).

I'm not entirely sure how scrollableFind works (I only scanned the code) but I think it's possible for those functions to not work (because the Dom is still loading), so container is still null, but then by the time you reach the last conditional, the dom might be loaded by then.

Thank you for webpack.config.js.

That makes sense. I think the following code as improvement. (It is the previous code to be compiled)

getContainer(selector, callback) {
  const { verticalScroll, horizontalScroll } = this.options;
  const finalCallback = callback.bind(this);
  let container = null;

  if (verticalScroll) {
    container = Dom.scrollableFind(selector, "y");
  }

  if (!container && horizontalScroll) {
    container = Dom.scrollableFind(selector, "x");
  }

  // Note: End if find a container.
  if (container) {
    finalCallback(container);

  // Note: If container not found, and DOM has not been built, to find the container.
  } else if (!isDomContentLoaded) {
    let isCompleted = false;

    const handleDomContentLoaded = () => {
      isCompleted = true;
      removeEvent(doc, DOM_CONTENT_LOADED, handleDomContentLoaded);
      this.getContainer(selector, callback);
    };

    const handleLoad = () => {
      removeEvent(win, LOAD, handleLoad);
      if (!isCompleted) {
        this.getContainer(selector, callback);
      }
    };

    addEvent(doc, DOM_CONTENT_LOADED, handleDomContentLoaded);
    addEvent(win, LOAD, handleLoad);

  } else {
    finalCallback(null);
  }
}

This seems to solve the previous problem. (I tried this code apply to the demo page. It is operating normally)

What do you think?

Let me test it, I will get back to you in two hours? Thank you for the quick response!

You're welcome!
It's getting late over here. I'll try to check tomorrow :)

I tested and this looks good to me.

Can you let me know if / when the Sweet Scroll library is updated with the above ^^ change?

Thanks so much for looking into this!

Thanks for test. And I intend to update sweet-scroll.js.
I will let you know after I released it. Please wait a little ๐Ÿ˜ƒ

Thank you, too!

A bug fix version has been released.
Please try it if you have the chance :)

sweet-scroll - npm

Hi, you are (I am guessing) sleeping, but since you will be awake when I am sleeping I will leave a note.

I tested this again and pushed it out to another environment, and it did not work (don't ask...it worked in the earlier environment I tested).

But I did notice this line:

else {
        finalCallback(null);
      }```

The problem with that is that it `finalCallback` (mostly) refers to this function:

```JS
function (target) {
      _this.container = target;
      _this.header = $(params.header);
      _this.tween = new ScrollTween(target);
      _this._trigger = null;
      _this._shouldCallCancelScroll = false;
      _this.bindContainerClick();
      _this.hook(params, "initialized");
    }```

If you say `finalCallback(null)` then the `target` will be null, which means the `container` will be set to null. Which means when I try to scroll using `toElement` it won't work (since the container is null).

Hope this makes sense and if you have time to take a look at in in the next few weeks that would be a super awesome help.

One question.
Do you specify a value for the second argument of SweetScroll constructor?
I would be happy if you know anything, please tell ;-)

I want to pursue the cause of the scrollable element not found bug.
thanks!

We tried the following (all different scenarios)

let sweetScroll = new SweetScroll({}, 'body')

let sweetScroll = new SweetScroll({}, document.querySelector('body'))

let sweetScroll = new SweetScroll({})

In each case, the SweetScroll always works when we don't minify our code. But then when we minify it breaks.

Now it works when I minify but it breaks when we move it to our staging environment. I think because it loads at a different speed again (some async condition between the dom being ready and the container getting initialized)

Again, thank you for taking the time to look at this.

My apologies for the late reply.
When do you execute they codes?

  • Immediately after loading of script
  • After DOMContentLoaded
  • After onload
  • Other...

Thank you for doing...so many times!

I'm using vuejs, it installs as a "vue plugin"...so I think before the DOMContentLoads.

I tried putting it in a domcontentload listener, but then I noticed that the sweet scroll plugin already listens for domloading?

Oh, that makes sense. sweet-scroll.js is listened for DOMContentLoaded.
Would you debug to scrollableFind processing?

PS: In addition, whether it would be better to create a plug-in of Vue.js if necessary?

scrollables for "y" is an empty array (length 0) for "selectors: body".

That happens twice.

For direction "x" that does not occur.

But also, when you use toElement if the container is null, then the scrolling won't work. If you have the final callback with null as a parameter, when can the container not be null after that?

For example, will you work the following code? (getContainer() method)

  getContainer(selector, callback) {
    const { verticalScroll, horizontalScroll } = this.options;
    const finalCallback = callback.bind(this);
    let container = null;

    if (verticalScroll) {
      container = Dom.scrollableFind(selector, "y");
    }

    if (!container && horizontalScroll) {
      container = Dom.scrollableFind(selector, "x");
    }

    if (container) {
      finalCallback(container);

    // Note: Check readyState
    } else if (!/comp|inter|loaded/.test(doc.readyState)) {
      let isCompleted = false;

      const handleDomContentLoaded = () => {
        removeHandlers();
        isCompleted = true;
        this.getContainer(selector, callback);
      };

      const handleLoad = () => {
        removeHandlers();
        if (!isCompleted) {
          this.getContainer(selector, callback);
        }
      };

      const removeHandlers = () => {
        removeEvent(doc, DOM_CONTENT_LOADED, handleDomContentLoaded);
        removeEvent(win, LOAD, handleLoad);
      };

      addEvent(doc, DOM_CONTENT_LOADED, handleDomContentLoaded);
      addEvent(win, LOAD, handleLoad);

    } else {
      setTimeout(() => this.getContainer(selector, callback), 1);
    }
  }

Do not listen of DOMContentLoaded in the sweet-scroll.js. It has been changed so as to determine the loading status of the DOM.

Do you have a built (ES5) version of that code I can drop in and test?

I tried to drop this into the src but I could not build (I think I am
missing some dependencies)

On 21 August 2016 at 23:43, tsuyoshi wada notifications@github.com wrote:

For example, will you work the following code? (getContainer() method)

getContainer(selector, callback) {
const { verticalScroll, horizontalScroll } = this.options;
const finalCallback = callback.bind(this);
let container = null;

if (verticalScroll) {
  container = Dom.scrollableFind(selector, "y");
}

if (!container && horizontalScroll) {
  container = Dom.scrollableFind(selector, "x");
}

if (container) {
  finalCallback(container);

// Note: Check readyState
} else if (!/comp|inter|loaded/.test(doc.readyState)) {
  let isCompleted = false;

  const handleDomContentLoaded = () => {
    removeHandlers();
    isCompleted = true;
    this.getContainer(selector, callback);
  };

  const handleLoad = () => {
    removeHandlers();
    if (!isCompleted) {
      this.getContainer(selector, callback);
    }
  };

  const removeHandlers = () => {
    removeEvent(doc, DOM_CONTENT_LOADED, handleDomContentLoaded);
    removeEvent(win, LOAD, handleLoad);
  };

  addEvent(doc, DOM_CONTENT_LOADED, handleDomContentLoaded);
  addEvent(win, LOAD, handleLoad);

} else {
  setTimeout(() => this.getContainer(selector, callback), 1);
}

}

Do not listen of DOMContentLoaded in the sweet-scroll.js. It has been
changed so as to determine the loading status of the DOM.

โ€”
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/tsuyoshiwada/sweet-scroll/issues/27#issuecomment-241308899,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAvEYl8eRkQUNZQzoJOi6Zw1IUG3XBYBks5qiRrOgaJpZM4JlboO
.

Sorry about that...
I pushed the code in a different branch. Please try to checkout it.

tsuyoshiwada/sweet-scroll at try-fix-issue-#27

This appears to work (locally). I tried setting the throlling on my network tab too.

Can you push it to master? Unfortunately (unless you know how) I can't 100% test because our staging environment builds from it's own package.json, so I think it needs to be in master. Unless you know how to make a package.json pull from a (non-master) branch instead??

Thanks for tests!
But, Can't still merge to master. Because It's to the failure to tests in mocha.

I think the npm doc would be helpful.

Got it...but so, should I wait for the mocha tests to pass first?

On 22 August 2016 at 09:30, tsuyoshi wada notifications@github.com wrote:

Thanks for tests!
But, Can't still merge to master. Because It's to the failure to tests in
mocha.

I think the npm doc
https://docs.npmjs.com/files/package.json#git-urls-as-dependencies
would be helpful.

โ€”
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/tsuyoshiwada/sweet-scroll/issues/27#issuecomment-241412663,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAvEYodfmN-J_EE0gXLAdHXwJYnJEH6iks5qiaRngaJpZM4JlboO
.

No, I think that priority test the work on staging environment. (Sorry about my poor English...)

So I tried

"dependencies": {
"sweet-scroll": "^1.0.4#21bd8a434f",

But I get this error:

npm ERR! No compatible version found: sweet-scroll@^1.0.4#21bd8a434f

npm ERR! Valid install targets:

On 22 August 2016 at 10:36, tsuyoshi wada notifications@github.com wrote:

No, I think that priority test the work on staging environment. (Sorry
about my poor English...)

โ€”
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/tsuyoshiwada/sweet-scroll/issues/27#issuecomment-241433181,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAvEYlSeDr8vdAwC8dPghvAp10Pg1528ks5qibQCgaJpZM4JlboO
.

Wait, sorry, got it working with:

"sweet-scroll": "git+https://github.com/tsuyoshiwada/sweet-scroll#21bd8a434f
",

Testing now.

On 22 August 2016 at 10:51, Nicole Chung nicole.chung@gmail.com wrote:

So I tried

"dependencies": {
"sweet-scroll": "^1.0.4#21bd8a434f",

But I get this error:

npm ERR! No compatible version found: sweet-scroll@^1.0.4#21bd8a434f

npm ERR! Valid install targets:

On 22 August 2016 at 10:36, tsuyoshi wada notifications@github.com
wrote:

No, I think that priority test the work on staging environment. (Sorry
about my poor English...)

โ€”
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/tsuyoshiwada/sweet-scroll/issues/27#issuecomment-241433181,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAvEYlSeDr8vdAwC8dPghvAp10Pg1528ks5qibQCgaJpZM4JlboO
.