Better npm/browserify support

[eirikurn]

Fix plugin dependencies.
Export ScrollMagic directly.
Support gsap’s strange exports.

[eirikurn]

It's not perfect but it's better, and plugins are now usable.

Example of use:

npm i --save scrollmagic
npm i --save gsap

// Load ScrollMagic
let ScrollMagic = require('scrollmagic');

// Load plugins
require('scrollmagic/scrollmagic/uncompressed/plugins/animation.gsap');
require('scrollmagic/scrollmagic/uncompressed/plugins/debug.addIndicators');

To make loading plugins nicer, they could be wrapped in the root directory somehow, or better yet, published in their own npm package.

[krnlde]

could be wrapped in the root directory somehow

you probably mean npm dedupe. I'm not very familiar with browserify, but using these advanced node techniques you're using - like ES6, CommonJS modules and browserify - need definitely more consideration.

[eirikurn]

Sorry, I wasn't talking about dedupe, that's actually not a problem since scrollmagic doesn't have a strict dependency on gsap (nor should it, since only an optional plugin depends on it).

I meant I would like to do this:

require('scrollmagic/plugins/animation.gsap');  // 1
// or
require('scrollmagic-gsap');                    // 2 - preferred

instead of

require('scrollmagic/scrollmagic/uncompressed/plugins/animation.gsap');

Note, nr 1 can be done with some prepublish script which flattens the package before it's sent to npm, or even simpler by having a dummy commonjs file named plugins/animations.gsap.js which requires the correct file like above.

Nr 2 can be done by splitting the plugins into their own repos and npm packages. If preferred, it would also be possible to publish multiple npm packages from this repo using some tricks.

[janpaepke]

Hi there and thanks for your help and input.
Sorry about the delayed answer, I was on holidays.

I was actually thinking about doing something like this as well, but wasn't sure how to work with the dependencies in the plugins. (I also don't work with browserify)
Apparently like this: https://github.com/janpaepke/ScrollMagic/pull/254/files#diff-9f7f3911ed5aab938b37c2bac61c1bf3R35
:)

Have you tested this?

I definitely prefer solution Nr1, because I want to keep all plugins organized in the same package.
I like the idea of changing the file structure with a post install script and I will look into that.
One question:
Do you think it even makes sense to publish a minified version to ScrollMagic to npm or are people who use npm doing their own minification/concatenation process anyway?

I would also like to ask you to change the pull request to only contain the files in the dev folder.
I think it's better to only commit the dist files with a new release.
This is also noted here: https://github.com/janpaepke/ScrollMagic/blob/master/CONTRIBUTING.md#pull-requests

thanks again and best regards,
J

[eirikurn]

There isn't really a hard dependency on gsap, the developer needs to install it manually for the gsap plugin to work. I can understand having the plugins in the same package, but that actually doesn't follow the spirit of npm. Separating them in npm would allow the gsap plugin to have an explicit dependency on gsap. You could still keep the code for all of the packages in this repo, and distribute them together for bower or downloading. Anyway, I do respect your opinion and this is good enough for now.

So this PR works for me. I am currently using ScrollMagic in a Browserify-based project, with gsap and debug plugins, exactly like the first code example.

I have not used or seen any client-side npm packages which are published with minified code. You need Browserify anyways which handles the concatenation, and minification is very easy from there. It is quite addicting to have an unminified source map of all project and library code in the browser.

Sorry about the dist files, that makes sense. I've rebased this PR to not include dist files. PTAL

[janpaepke]

I can understand having the plugins in the same package, but that actually doesn't follow the spirit of npm. Separating them in npm would allow the gsap plugin to have an explicit dependency on gsap.

I hear ya, but ScrollMagic wasn't originally intended as an npm package.
npm support was actually just added with 2.0.0
We struggled with this decision for a while, because in my opinion npm is a commonjs package manager, so for backend stuff. Frontend is bower (imho).
But apparently (and evidently) there are lots of people who disagree and use npm for everything.
That is why we decided to publish for npm as well.
The reason I'm hesitant to publish in separate packages is for the simple reason of losing track.
I like that now it's neatly packaged and centralized.

It is for my lack of knowledge here that I am unsure if I should kick the minified source from the npm package. The reason is even if it is true, that people who use npm always do their own minification (through browserify or other) they will miss several optimizations that the minified files from the dist have.
For example the removal of logging functions.
For more info see here:

ScrollMagic/gulpfile.js

Lines 209 to 224 in 5c7fae2

	.pipe(replace({
	patterns: [
	{ // replace throw messages in scene option validations
	match: /^\s*throw \[.+\];\s*$/gm,
	replacement: 'throw 0;'
	},
	{ // remove log messages
	match: /((\s*.+\._?)|(\s+))log\([0-3],.+\)\s*;\s*$/gm,
	replacement: ''
	},
	{ // remove unnecessary stuff in minify
	match: /\/\/ \(BUILD\) - REMOVE IN MINIFY - START[^]*?\/\/ \(BUILD\) - REMOVE IN MINIFY - END/gm,
	replacement: ''
	}
	]
	}))

So what's the opinion here?

[eirikurn]

That's actually a very good point, about the optimizations. I haven't actually noticed how other client-side libraries deal with code changes between dev and prod versions. I think few people care and some don't mind dead debug code in their bundles (assuming it can be turned off at runtime).

Maybe predefines and dead code removal in UglifyJS can handle this in a more standard way. Then those that care, can configure it manually. I do realise that's not optimal.

// near top of Scrollmagic.js and plugins.
if (typeof SCROLLMAGIC_DEBUG === 'undefined') SCROLLMAGIC_DEBUG = true;

// later
if (SCROLLMAGIC_DEBUG) {
  function log() {...}
}

// In gulpfile.js minified task
.pipe(uglify({compress: {global_defs: {SCROLLMAGIC_DEBUG: false}}}))`

[janpaepke]

Well this solution would still mean that all the debug text information (which are strings) would be exported to the minified file, making it unnecessarily big.
The functionality you outlined actually already exists, using the loglevel variable, which can be used to suppress all console messages.
In the minified version they aren't even exported in the first place.

Long story short I think the best way to go is flattening the file structure, as suggested and leave both the regular and minified files in there.

It should be the same as it currently is on cdnjs, where ScrollMagic.js and ScrollMagic.min.js are in the root folder and all plugins (minified and unminified) reside in /plugins.

This should be done using a postinstall script.

Do you agree?
@krnlde what are your thoughs on this?

Another question that remains is how to treat the package.json.
Should it link to scrollmagic/uncompressed/ScrollMagic.js as the main file or ScrollMagic.js, so the location it would have after the postinstall script ran.
Or should the postinstall script modify the package.json?

[krnlde]

It should be the same as it currently is on cdnjs, where ScrollMagic.js and ScrollMagic.min.js are in the root folder and all plugins (minified and unminified) reside in /plugins.

I'm with you.

Tough, I think there must be a better solution for the future. At the moment of reading I thought that "plugin" is probably not the best word describing what is in this folder. Since the actual plugins are loaded via //cdnjs.cloudflare.com/ajax/libs/gsap/1.16.1/TweenMax.min.js et al, these files behave more like "adapters" for the plugins. Or am I missing sth?

[janpaepke]

Mh. They are indeed more like "extensions".
But GSAP for example follows the same structure.
It has the main files in the main folder and "extensions" of functionality in the "plugin" folder.
Alson one that might be considered an "adapter" (the raphael plugin).
In jQuery plugins are also basically extensions of basic functionality of $.
So I think the naming should be okay?

[eirikurn]

Hm, I'm not sure if you understood. Using my example, uglify would remove the whole if clause since it's dead code, so nothing inside it would show up in minified code.

For flattening, another option is to build the whole npm package structure into a subfolder with gulp, including a correct package json file, and publish from there. NPM recommends using prepublish scripts instead of postinstall scripts to keep the package tar ready to consume with minimal cruft and dependencies (don't want to test the postinstall script on all target OS's).

[janpaepke]

Hm, I'm not sure if you understood. Using my example, uglify would remove the whole if clause since it's dead code, so nothing inside it would show up in minified code.

Ah, I see now. I didn't know uglify did that.
That would certainly be a way to go, but would require users to know about this.
Is this common practice?

Regarding publishing this is also a very good Idea.
So I'd basically add a gulp task that does the npm publishing.
it creates a flattened folder with adapted package.json and the js files.
Then it publishes to NPM and removes the folder from the package.

What do you think?

[eirikurn]

That would certainly be a way to go, but would require users to know about this.
Is this common practice?

I don't think this is a commonly used feature in browserify. Nor do I think it's commonly used to fix this problem of supporting both dev/dist code in npm, it's not very intuitive for users as you say.

But I do think it's a smart way to skip code in production, and useful to both libraries and size-sensitive websites.

What do you think?

The publishing plan sounds really good to me.

[janpaepke]

So here's what I decided:
I'll leave both the minified and unminified in the npm package.
The workaround using the variable is to unknown for anybody to actually use it.
If people care about size optimization they will use minified version, if not they'll auto-minify.

Publishing logic:
I created two new gulp tasks, see here:

ScrollMagic/gulpfile.js

Lines 308 to 326 in fcc3255

	gulp.task("npm:prepublish", [], function () {
	// update package.json
	gulp.src("./package.json")
	.pipe(jeditor({main: "ScrollMagic.js"}, {keep_array_indentation: true}))
	.pipe(gulp.dest("./"));
	// copy dist files to root.
	return gulp.src(options.folderOut + "/" + options.subfolder.uncompressed + "/**/*.js")
	.pipe(gulp.src(options.folderOut + "/" + options.subfolder.minified + "/**/*.js"))
	.pipe(gulp.dest("./"));
	});
	gulp.task("npm:postpublish", [], function (callback) {
	// update package.json
	gulp.src("./package.json")
	.pipe(jeditor({main: path.relative("./", options.folderOut + "/" + options.subfolder.uncompressed + "/ScrollMagic.js")}, {keep_array_indentation: true}))
	.pipe(gulp.dest("./"));
	// remove root dist files and plugin folder
	del(["./ScrollMagic*.js", "./plugins"], callback);
	});

One is executed pre-publish and one post-publish.
The prepublish one copies the dist files to the root folder and modifies the package.json accordingly.
The postpublish one basically reverts to the original state.

I can't test it now, because the version number would need to be bumped for that, but we'll see if it works, when that happens.

Any input?

[eirikurn]

I like it. Especially that the old require paths will still work.

I think there is just one more issue, and that's how the plugins find ScrollMagic. They're now doing require('scrollmagic') which I don't think is recommended, since it basically goes outside the package to search for itself. Though this happens to work when it's npm installed in another project.

But I think we want to make sure that:

1. Since plugins are wrapped inside the same package as scrollmagic, they should use a relative require path to find scrollmagic.
2. If you require a minified plugin, it should require the minified scrollmagic so you don't end up with both minified and uncompressed scrollmagic in your bundle.
3. Figure out if you want to (officially) support the old paths, with relative requires. As that will also load an extra copy of scrollmagic unless we do something smart.

I'll think some more if there is a simple way to achieve this.

[janpaepke]

I like it. Especially that the old require paths will still work.

actually they won't.
The ./scrollmagic folder wouldn't be included anymore.
This also answers your number 3). I don't think the files should be duplicated in the published package.
This will only cause confusion.

Regarding your 1) and 2) I can't really make an informed decision, because I lack knowledge here.
What are the conventions and best practices?
We can't be the first ones stumbling over this?

[hew]

Any chance we can open this back up? None of the suggestions are working, and not running this through browserify or webpack in 2015 is insane.

[masonbraun]

^^^^^^

[nevace]

+1

[iamdash]

+1

[chay22]

I wonder if there's any changes made on this. Still can't make it working with browserify

[mbaierl]

Unfortunately it still does not work as expected....

[codeofsumit]

asking again in 2018