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

UMD-compatible structure #98

Closed
wants to merge 2 commits into from
Closed

Conversation

dpogue
Copy link

@dpogue dpogue commented May 1, 2017

Continuing from #87:

The issue is that TypeScript doesn't output UMD modules that are available as browser globals. So instead, this PR uses TypeScript to output ES6 modules which are wrapped up with rollup to produce a UMD module that works everywhere.

I've been using this branch in a TypeScript/WebPack project for a while with no issues.

@timruffles
Copy link
Owner

That's great, thanks so much! 🎉

How close is everyone being happy with this being v2, and releasing this to take it out of beta?

Copy link
Collaborator

@reppners reppners left a comment

Choose a reason for hiding this comment

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

Overall I'm not a fan to remove the development server and release helper tasks defined in Gruntfile without alternative solution. The scope of this PR should be to enable UMD support without removing the current development and release workflow. Also we need to make sure the Uglify settings are not lost.


// default task for developers to start coding
grunt.registerTask("default", ["connect:dev", "watch"]);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing the Gruntfile removes valuable development helpers, release helpers and the special uglify config which raises minified size. I'd rather keep the Gruntfile and invoke Rollup from there.

//</editor-fold>


export const DragDropPolyfill = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Re-export is not really needed.

velocityFn:( velocity:number, threshold:number ) => number;
}

function SetOptions( options:ScrollOptions ):void {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Directly export this function rather than have a DragDropPolyfill reexport.

}

export const HandleDragImageTranslateOverride:DragImageTranslateOverrideFn = handleDragImageTranslateOverride;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this should get a specific "scroll" related name to avoid conflicts on the global "DragDropPolyfill" namespace.

// regardless of whether that event was canceled or not.
//this.dragenter( newTarget, this.currentDropTarget );
//this.currentDropTarget = newTarget;

Copy link
Collaborator

Choose a reason for hiding this comment

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

the definition of this export wrapper will result in global usage pattern of

/// <reference path="../../src/drag-drop-polyfill.d.ts" />

DragDropPolyfill.DragDropPolyfill.Initialize();

Rather just export the Initialize() function to get global usage like

/// <reference path="../../src/drag-drop-polyfill.d.ts" />

DragDropPolyfill.Initialize();

Module usage

import {Initialize} from "drag-drop-polyfill";

Initialize();

or

import {Initialize as DragDropPolyfill} from "drag-drop-polyfill";

DragDropPolyfill();

@reppners reppners mentioned this pull request May 25, 2017
3 tasks
@reppners
Copy link
Collaborator

reppners commented Jun 4, 2017

closing in favor of #100

@reppners reppners closed this Jun 4, 2017
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.

3 participants