Break up utility functions into smaller libraries #2139

heff · 2015-05-11T22:26:09Z

Getting started on this one but interested in feedback early on.

I've started by just breaking up lib into libraries (e.g. dom.js), and maintaining the Dom.createEl library object property style. The next piece will be determining how to pull in individual functions instead of the entire library.

If you look at lodash modularized or npm dom the examples just have you name the function variable the same as simple function name.

var first = require('lodash.first');

I'm not super comfortable with that as a standard.

It's not obvious what's internal vs. external to the file
The function names get pretty generic and I think it could get confusing or even cause conflicts with other functions on the page.

An example on the second one is Component createEl. Babel creates named functions for class methods and I wonder if the createEl named function for the method would overwrite the createEl that's been imported.

My best thoughts are finding some common prefix for functions that are imported.

$createEl()
uCreateEl() // u for util
domCreateEl()

I'm not in love with any of those options but could live with any of them.

Anyone seen or used better options? Or are most people fine with the simple variable names.

heff · 2015-05-14T19:24:21Z

Anyone know what might have changed with source maps recently? Travis is failing on them.

heff · 2015-05-15T22:23:08Z

Fixed source maps error by locking down grunt-browserify version to 3.5.1 (it's now at 3.8.0). Probably not good long term.

(ping @forbesjo)

heff · 2015-05-15T22:27:20Z

@videojs/core-committers can I get someone to glance at this? Now would be a good time to merge it in to prevent big merge conflicts. It's probably most valuable to look at the commit messages. The code itself didn't get changes all that much, just got moved around.

The next step is to switch the wildcard import * as X instances to pull in the specific functions. That could be another PR though.

mmcc · 2015-05-15T23:06:41Z

src/js/utils/browser.js

+import document from 'global/document';
+import window from 'global/window';
+
+const USER_AGENT = window.navigator.userAgent;


export const USER_AGENT = ...

USER_AGENT isn't used externally so I left it private.

there are a few other uses of the user agent, should we change those to use browser.USER_AGENT instead, maybe?

mmcc · 2015-05-15T23:24:17Z

👍 lgtm

Broke out bind, guid, and element data functions from Lib Separated out more dom functions in to dom.js Broke out URL functions into url.js Removed setLocalStorage since it wasn't being used Moved browser tests out of lib Moved log functions into their own file Removed trim() since it wasn't being used Moved formatTime into its own file Moved round into its own file and renamed roundFloat() Moved capitalize into its own file and renamed as toTitleCase() Moved createTimeRange into its own file Removed Lib.arr.forEach infavor of the native forEach Removed Lib.obj.create in favor of native Object.create (ES6-sham) Removed obj.each in favor of native Object.getOwnPropertyNames().forEach() Removed obj.merge and copy. Using lodash.assign instead. Replaced Lib.obj.isPlain with lodash.isPlainObject Removed Lib.obj.isArray in favor of the native Array.isArray Also removed the lib.js tests file as all tests have been moved or removed. Removed Lib.isEmpty in favor of !Object.getOwnPropertyNames().length Switched Util.mergeOptions and deepMerge to use new mergeOptions() Moved Lib.TEST_VID to Html5.TEST_VID Removed Lib references everywhere. Woo! Attempting to fix sourcemap test errors by setting grunt-browserify version Switched to object.assign from lodash.assign Removed unused 'inherits' dependency Reorganzied test files and added '.test' to file names Combined js/core.js and js/video.js Moved events.js into the utils directory

gkatsev · 2015-05-18T18:31:57Z

Looking over this now. We can always make new PRs if there's anything we want to change.

As for the naming, we shouldn't do any special naming. From a functionality perspective, there shouldn't be a different between internal and external functions. However, I've seen styleguides that suggest (or require, eslint might be able to help with this, though, not sure) that the imports at the top of the file are grouped by where they're coming from.
I.e., the very top group of imports are built-ins, then come the group of imports from npm, and then come the imports that are local to the project.

gkatsev · 2015-05-18T18:33:01Z

package.json

@@ -24,8 +24,11 @@
  "style": "./dist/video-js.css",
  "dependencies": {
    "global": "^4.3.0",
+    "lodash.clonedeep": "^3.0.0",
+    "lodash.isplainobject": "^3.0.2",
+    "lodash.merge": "^3.2.1",


is this package used anymore?

Yeah, it's used in mergeOptions

gkatsev · 2015-05-18T19:00:05Z

Do we require es5-sham for other babelify things? If not, I think we should still make our own shims for Object.create and Object.getOwnPropertyNames so that only es5-shim is necessary.

Anyway, mostly minor stuff. Looks good!

heff · 2015-05-18T19:51:27Z

Yeah, so far the sham (as far as I know) is only needed for Object.create in the es6 classes. I'd also prefer to only need the shim, but I don't know how we'd make babel use our own Object.create shim.

gkatsev · 2015-05-18T19:53:18Z

Yeah, if babel requires it, that's fine. We could theoretically make another transform that removes uses of Object.create and replaces it with a shim.

gkatsev · 2015-05-18T19:56:00Z

src/js/utils/url.js

@@ -0,0 +1,89 @@
+import document from 'global/document';


we should evaluate whether the built-in url module gives us everything we care about.

heff · 2015-05-18T20:13:46Z

make another transform that removes uses of Object.create and replaces it with a shim

Hmm...that's an interesting idea. I'm on the fence of whether or not it's worth it. We'd still need to bundle the es5 shim with the html5 video shim, so I can't tell if that would remove complication or add more. @mmcc @dmlap, any other opinions on whether we should go that route to avoid needing the sham?

dmlap · 2015-05-20T04:11:44Z

I think I'd prefer a minimal shim for what we need if we can get away with it. I'm not an es5-sham expert though, so factor in this opinion is coming from a place of great ignorance.

heff added the enhancement label May 11, 2015

heff force-pushed the breakup branch from bed8670 to 26ae381 Compare May 14, 2015 06:04

mmcc reviewed May 15, 2015
View reviewed changes

heff force-pushed the breakup branch from e0b33e1 to 8fe5b38 Compare May 15, 2015 23:55

heff added the confirmed label May 16, 2015

heff force-pushed the breakup branch from 3675675 to a8ff970 Compare May 16, 2015 01:22

heff merged commit a8ff970 into videojs:master May 16, 2015

heff mentioned this pull request May 16, 2015

Breaking apart files #2010

Closed

gkatsev reviewed May 18, 2015
View reviewed changes

heff mentioned this pull request May 27, 2015

Expose utility functions #1232

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Break up utility functions into smaller libraries #2139

Break up utility functions into smaller libraries #2139

heff commented May 11, 2015

heff commented May 14, 2015

heff commented May 15, 2015

heff commented May 15, 2015

mmcc May 15, 2015

heff May 15, 2015

gkatsev May 18, 2015

mmcc commented May 15, 2015

gkatsev commented May 18, 2015

gkatsev May 18, 2015

heff May 18, 2015

gkatsev commented May 18, 2015

heff commented May 18, 2015

gkatsev commented May 18, 2015

gkatsev May 18, 2015

heff commented May 18, 2015

dmlap commented May 20, 2015

Break up utility functions into smaller libraries #2139

Break up utility functions into smaller libraries #2139

Conversation

heff commented May 11, 2015

heff commented May 14, 2015

heff commented May 15, 2015

heff commented May 15, 2015

mmcc May 15, 2015

Choose a reason for hiding this comment

heff May 15, 2015

Choose a reason for hiding this comment

gkatsev May 18, 2015

Choose a reason for hiding this comment

mmcc commented May 15, 2015

gkatsev commented May 18, 2015

gkatsev May 18, 2015

Choose a reason for hiding this comment

heff May 18, 2015

Choose a reason for hiding this comment

gkatsev commented May 18, 2015

heff commented May 18, 2015

gkatsev commented May 18, 2015

gkatsev May 18, 2015

Choose a reason for hiding this comment

heff commented May 18, 2015

dmlap commented May 20, 2015