waypoints icon indicating copy to clipboard operation
waypoints copied to clipboard

CommonJS modules

Open summerisgone opened this issue 8 years ago • 15 comments

Here's my attempt to rewrite code in CommonJS modules. I've been using your waypoints a long time ago, and I'm happy to contribute.

This PR should solve issue #458. I didn't put files in lib/* because I don't like store code twice in repository. They can be easily added later.

summerisgone avatar Feb 28 '16 20:02 summerisgone

+1 Is this going to be pulled yet? @imakewebthings

TheLarkInn avatar Feb 29 '16 22:02 TheLarkInn

+1, totally would save me time

nstanard avatar Feb 29 '16 22:02 nstanard

On first reading this looks good @summerisgone. Thank you. I'm currently in the middle of caring for a newborn so my time here comes in sporadic, short chunks. I'll try my best to use those chunks to test and merge this over the next few days.

imakewebthings avatar Mar 01 '16 03:03 imakewebthings

@imakewebthings my congrats for your baby! Stay happy and healthy! I'm ready to discuss any solution, I've tried to keep compatibility with previous builds as much as possible.

summerisgone avatar Mar 01 '16 05:03 summerisgone

@summerisgone The files you added to lib, the test builds, did you edit them by hand after generating via npm run build? I get some mostly minor differences when I build locally and want to make sure I'm not missing something.

imakewebthings avatar Mar 04 '16 16:03 imakewebthings

@imakewebthings as I said, I excluded lib/* files from commit, to show only code difference for convenience. Added them now.

summerisgone avatar Mar 04 '16 17:03 summerisgone

@summerisgone Here's the plan. I've merged this PR into a branch, 5.0. I very much like this PR, and it makes it painfully obvious to me that a couple things should change in concert with this CommonJS changeset. The key ones:

  1. No more separate shortcut files. Include it all into the main file, as you've done here. The downside of larger file size for things that not everyone uses is outweighed by the ease of use that comes with one simple file/require. This also leaves the door open for a custom build process in the future for the extremely bit-conscious users.
  2. No more jQuery/Zepto deps. That means in the shorcuts, and those two adapters can die. The extensions can still live on, but I believe it would be best to package it as a method one can call to decorate jQuery.fn. Like so:
Waypoint.extendJquery(jQuery)
jQuery('.blah').waypoint(...)

I intend to do that work on top of your changes in the 5.0 branch. Simultaneously, there are some bug fixes and such that I would like to release as 4.x following master. Those changes will be folded into 5.0 through rebasing as I go along.

imakewebthings avatar Mar 08 '16 06:03 imakewebthings

@imakewebthings thank you for submission, I'm very glad again to contribute :) I have some questions about your plan:

  1. I didn't get, do you want to make all-in-one build or leave separate files? As third opinion, you can build two versions like full and minimal. I leaved shortcuts builds as is, and they depends on global.Waypoint so they can be just added on the page.
  2. How do you plan to get rid from jQuery/Zepto deps, if some [useful] shortcuts depends on them? I can try to rewrite code to var jquery = require('jquery') rather than using window.jQuery. But then the module (in CommonJS context) would depend on jquery, which lays to some overweight (so I leaved them as optional dependencies and used global context for variables).

summerisgone avatar Mar 08 '16 11:03 summerisgone

@summerisgone The package.json main would point to an all-in-one build. If somebody were worried about file size and wanted to only include a subset of everything, there's nothing stoppng them from using require('waypoints/src/whatever.js') and building that into their final bundle. The all-in-one mostly makes things 100x easier to explain. I'm also not a fan of leaking Waypoint into the global namespace if a user is going the CommonJS route. Having the separate files would necessitate that or some other means of extending the Waypoint namespace.

For the jQuery/Zepto shortcuts, I intend to update those shortcuts to work without jQuery/Zepto.

imakewebthings avatar Mar 08 '16 23:03 imakewebthings

Awesome plan @imakewebthings I hope it will become reality soon.

MartinMuzatko avatar Jul 05 '16 12:07 MartinMuzatko

I agree so much. Waypoints is awesome, but using it with Webpack etc. can be a pain in the first point. CommonJS would solve it!

axe312ger avatar Oct 19 '16 15:10 axe312ger

If you need assistance from us @ webpack ensuring this can be bundled properly let me know. We are willing to help.

TheLarkInn avatar Oct 19 '16 18:10 TheLarkInn

Thank you for the offer @TheLarkInn, but the following official workaround is working pretty well for me. :)

However, I would strongly vote for switching to a wider supported module in the next future. Maybe even consider es-modules via babel to supply all common module formats out there.

Here is the workaround in case somebody else is looking for a fix:

plugins: [
  new webpack.ProvidePlugin({
    $: 'jquery',
    jQuery: 'jquery',
    'window.jQuery': 'jquery'
  })
]

axe312ger avatar Oct 19 '16 22:10 axe312ger

@axe312ger a PR on the readme would be really beneficial.

TheLarkInn avatar Oct 20 '16 00:10 TheLarkInn

@summerisgone Here's the plan. I've merged this PR into a branch, 5.0. I very much like this PR, and it makes it painfully obvious to me that a couple things should change in concert with this CommonJS changeset. The key ones:

  1. No more separate shortcut files. Include it all into the main file, as you've done here. The downside of larger file size for things that not everyone uses is outweighed by the ease of use that comes with one simple file/require. This also leaves the door open for a custom build process in the future for the extremely bit-conscious users.
  2. No more jQuery/Zepto deps. That means in the shorcuts, and those two adapters can die. The extensions can still live on, but I believe it would be best to package it as a method one can call to decorate jQuery.fn. Like so:
Waypoint.extendJquery(jQuery)
jQuery('.blah').waypoint(...)

I intend to do that work on top of your changes in the 5.0 branch. Simultaneously, there are some bug fixes and such that I would like to release as 4.x following master. Those changes will be folded into 5.0 through rebasing as I go along.

Does that mean, that by default Waypoint won't use jQuery/Zepto or is it just the shortcuts?

Berkmann18 avatar Jan 07 '20 13:01 Berkmann18