Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CommonJS modules #481

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

summerisgone
Copy link

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.

@TheLarkInn
Copy link

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

@nstanard
Copy link

+1, totally would save me time

@imakewebthings
Copy link
Owner

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.

@summerisgone
Copy link
Author

@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.

@imakewebthings
Copy link
Owner

@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.

'use strict'

var $ = window.jQuery
var Waypoint = window.Waypoint
var jquery = global.jQuery //require('jquery')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the comment here a remnant of an attempt to load jQuery via require, or a reminder to change it to require later at some point, or something else entirely?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to require jquery, but jquery's npm package doesn't contain $.forEach utilities and stuff. So this comment is more redundant. I still can try to shim jquery import.

@summerisgone
Copy link
Author

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

@imakewebthings
Copy link
Owner

@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.

@summerisgone
Copy link
Author

@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).

@imakewebthings
Copy link
Owner

@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.

@MartinMuzatko
Copy link

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

@axe312ger
Copy link

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!

@TheLarkInn
Copy link

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

@axe312ger
Copy link

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'
  })
]

@TheLarkInn
Copy link

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

@Berkmann18
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants