container is not set when code is compressed with webpack #27

nicolechung · 2016-08-16T14:02:59Z

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);
        }
      }

The text was updated successfully, but these errors were encountered:

nicolechung · 2016-08-16T14:04:14Z

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?

wadackel · 2016-08-16T14:52:59Z

Hi @nicolechung. Thanks you for issues.
I will confirm that afterward 😃

wadackel · 2016-08-17T13:26:51Z

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?

nicolechung · 2016-08-17T14:04:20Z

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

nicolechung · 2016-08-17T14:19:07Z

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.

wadackel · 2016-08-17T15:39:47Z

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?

nicolechung · 2016-08-17T15:52:43Z

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

wadackel · 2016-08-17T16:04:15Z

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

nicolechung · 2016-08-18T14:43:52Z

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!

wadackel · 2016-08-18T15:49:14Z

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!

Fix issue #27

wadackel · 2016-08-18T16:41:32Z

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

sweet-scroll - npm

nicolechung · 2016-08-18T19:00:42Z

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.

wadackel · 2016-08-19T04:47:14Z

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!

nicolechung · 2016-08-19T14:35:55Z

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.

wadackel · 2016-08-21T16:55:23Z

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!

nicolechung · 2016-08-21T16:57:42Z

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?

wadackel · 2016-08-22T01:10:05Z

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?

nicolechung · 2016-08-22T01:24:12Z

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

That happens twice.

For direction "x" that does not occur.

nicolechung · 2016-08-22T01:37:57Z

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?

wadackel · 2016-08-22T03:43:09Z

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.

nicolechung · 2016-08-22T12:26:28Z

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
.

wadackel · 2016-08-22T12:48:15Z

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

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

nicolechung · 2016-08-22T12:57:53Z

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??

wadackel · 2016-08-22T13:30:14Z

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.

nicolechung · 2016-08-22T13:53:42Z

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
.

wadackel · 2016-08-22T14:36:49Z

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

nicolechung · 2016-08-22T14:51:45Z

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
.

nicolechung · 2016-08-22T15:07:22Z

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
.

wadackel pushed a commit that referenced this issue Aug 18, 2016

Fix issue #27

3403f93

wadackel mentioned this issue Aug 18, 2016

Fix issue #27 #28

Merged

wadackel pushed a commit that referenced this issue Aug 18, 2016

Merge pull request #28 from tsuyoshiwada/issue-#27

51b72e3

Fix issue #27

wadackel pushed a commit that referenced this issue Aug 22, 2016

Try to fix for issue-#27

21bd8a4

nicolechung closed this as completed Sep 18, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

container is not set when code is compressed with webpack #27

container is not set when code is compressed with webpack #27

nicolechung commented Aug 16, 2016 •

edited

nicolechung commented Aug 16, 2016

wadackel commented Aug 16, 2016

wadackel commented Aug 17, 2016

nicolechung commented Aug 17, 2016

nicolechung commented Aug 17, 2016

wadackel commented Aug 17, 2016

nicolechung commented Aug 17, 2016

wadackel commented Aug 17, 2016

nicolechung commented Aug 18, 2016 •

edited

wadackel commented Aug 18, 2016

wadackel commented Aug 18, 2016

nicolechung commented Aug 18, 2016

wadackel commented Aug 19, 2016

nicolechung commented Aug 19, 2016

wadackel commented Aug 21, 2016

nicolechung commented Aug 21, 2016

wadackel commented Aug 22, 2016

nicolechung commented Aug 22, 2016

nicolechung commented Aug 22, 2016

wadackel commented Aug 22, 2016

nicolechung commented Aug 22, 2016

wadackel commented Aug 22, 2016

nicolechung commented Aug 22, 2016

wadackel commented Aug 22, 2016

nicolechung commented Aug 22, 2016

wadackel commented Aug 22, 2016

nicolechung commented Aug 22, 2016

nicolechung commented Aug 22, 2016

container is not set when code is compressed with webpack #27

container is not set when code is compressed with webpack #27

Comments

nicolechung commented Aug 16, 2016 • edited

nicolechung commented Aug 16, 2016

wadackel commented Aug 16, 2016

wadackel commented Aug 17, 2016

nicolechung commented Aug 17, 2016

nicolechung commented Aug 17, 2016

wadackel commented Aug 17, 2016

nicolechung commented Aug 17, 2016

wadackel commented Aug 17, 2016

nicolechung commented Aug 18, 2016 • edited

wadackel commented Aug 18, 2016

wadackel commented Aug 18, 2016

nicolechung commented Aug 18, 2016

wadackel commented Aug 19, 2016

nicolechung commented Aug 19, 2016

wadackel commented Aug 21, 2016

nicolechung commented Aug 21, 2016

wadackel commented Aug 22, 2016

nicolechung commented Aug 22, 2016

nicolechung commented Aug 22, 2016

wadackel commented Aug 22, 2016

nicolechung commented Aug 22, 2016

wadackel commented Aug 22, 2016

nicolechung commented Aug 22, 2016

wadackel commented Aug 22, 2016

nicolechung commented Aug 22, 2016

wadackel commented Aug 22, 2016

nicolechung commented Aug 22, 2016

nicolechung commented Aug 22, 2016

nicolechung commented Aug 16, 2016 •

edited

nicolechung commented Aug 18, 2016 •

edited