scratchfoundation / scratch-www

Standalone web client for Scratch
https://scratch.mit.edu
BSD 3-Clause "New" or "Revised" License
1.59k stars 841 forks source link

Audit npm dependencies #564

Open thisandagain opened 8 years ago

thisandagain commented 8 years ago

/cc @mewtaylor @rschamp

hyperobject commented 8 years ago

Things that should be removed:

Things that shouldn't be removed

Most other packages in package.json are necessary for bundling and compilation.

mewtaylor commented 8 years ago

@technoboy10 thanks so much for this! Very helpful. Here're some thoughts:

Thoughts @rschamp @thisandagain ?

rschamp commented 8 years ago

@technoboy10 Thanks so much for going through this for us!

  • lodash.defaults - Object.assign should behave in the same way, but due to a few quirks of Object.assign and lodash.defaults, removing this dependency is actually more work than it's worth, IMO.

Could you elaborate @technoboy10 — which quirks?

  • google-libphonenumber, react-telephone-input - see react-modal

I fully agree about google-libphonenumber, however I think we probably could build react-telephone-input as our own component in order to eliminate a dependency. I used this component in the interest of time, but it left a bad taste in my mouth since I had to upgrade to React 15 in order for it to install, which made me have to upgrade a bunch of our other dependencies — that's the kind of silliness too many dependencies can cause.

+1 to building our own formsy-react components over using formsy-react-components. formsy-react-components expects you to build forms like you do in Bootstrap, but we aren't doing that. Not being able to change the order of help text, for example, is annoying.

On that note, formsy-react works pretty well for what we needed in the moment, but do you guys think it works in the ideal way? I like the way validation is configured for formsy-react inputs via props. However I don't like that it requires all inputs to be validated to be inside of a special formsy-react Form component (or that this component uses the poorly documented React context API). IMO the input should be able to handle validation by itself, even if it's not in a form. If you need to validate before submitting, or validate based on the values of different inputs, then that should be configurable via a higher order component. I didn't find anything that worked as flexibly as I'd like. I think there's still room for finding an alternative to formsy-react or building our own forms library. Or extending formsy-react to work this way and writing a convincing PR.

+1 also to replacing slick-carousel with our own component. I don't think the scope of what the component does is worth the overhead of the external dependency.

  • react-onclickoutside - the general concept isn't able to be replicated easily with built in JS features, but there may be a better package that does roughly the same thing (react-onclick-outside)

I didn't realize we're still using this as a mixin — I tried to get rid of all of our uses of mixins. I think it'd be worth upgrading this package to the latest version which changes the API to work as a higher order component.

  • keyMirror: Not really necessary, as the functionality can easily be replicated in vanilla JS (it just returns an object with the values being the same string as the key), but ergonomically it makes sense to keep.

Just for the record, the main reason for using this is that it's an enum compatible with aggressive minifiers. I don't think we're minifying in a way that requires this yet, but I don't think there's much risk to keeping this since it has no dependencies.

hyperobject commented 8 years ago

@rschamp The biggest quirk of Object.assign is the way it handles arguments. lodash.defaults({a: 'b', c: 'd'}, {a: 'e'}); returns {a: 'b', c: 'd'}, while Object.assign({a: 'b', c: 'd'}, {a: 'e'}); returns {a: 'e', c: 'd'}.

Additionally, lodash.defaults does not mutate objects passed into it, while Object.assign does mutate objects passed into it.

mewtaylor commented 8 years ago

@rschamp fwiw I am fine with keeping react-formsy at the moment. I can see what you mean about the input validating itself in the context of project/studio text areas, but I'd be more interested in at least exploring the idea of submitting an upstream PR before refactoring the whole thing with a new library (or based on our own implementation, which would take a lot of boilerplate). It kind of reminds me of the form class in Django the way it's currently set up, which is something we do use for single-input forms currently in a couple places.

rschamp commented 8 years ago

The only difference I see between Object.assign and lodash.defaults is the order the source object attributes are applied, but they can achieve the same thing. The way we use lodash.defaults, we expect arguments to be mutated, e.g. https://github.com/LLK/scratch-www/blob/99c76ee3bfd29e8ffb05cc7fc9a987019a7cef66/src/lib/api.js#L18-L23, so I think they're equivalent in that way.

I don't think you can use the mutating method of Object.assign in the same way as lodash.defaults, since the target values will always be overwritten by the source (rather than just treating them as defaults). E.g.,

var opts = {a: 1, b: 2, c:3};
defaults(opts, {a: 'a', b: 'b', c: 'c', d: 'd'});
// {a: 1, b: 2, c: 3, d: 'd'}
// opts = {a: 1, b: 2, c: 3, d: 'd'}

Object.assign(opts, {a: 'a', b: 'b', c: 'c', d: 'd'});
// {a: 'a', b: 'b', c: 'c', d: 'd'}
// opts = {a: 'a', b: 'b', c: 'c', d: 'd'}

Object.assign({a: 'a', b: 'b', c: 'c', d: 'd'}, opts);
// {a: 1, b: 2, c: 3, d: 'd'}
// opts = {a: 1, b: 2, c:3} (unchanged)

// Equivalent to lodash.defaults
opts = Object.assign({a: 'a', b: 'b', c: 'c', d: 'd'}, opts);
// {a: 1, b: 2, c: 3, d: 'd'}
// opts = {a: 1, b: 2, c: 3, d: 'd'}

But to me, this is actually a good thing about Object.assign, because functions that mutate arguments can lead to unexpected behavior.

I am for removing lodash.defaults in favor of Object.assign if we can achieve the same thing with both (in general, I'm for replacing any dependency with an equivalently convenient Javascript implementation if possible). Then whenever we use this functionality, we won't have to install a dependency in every codebase. This is with the understanding that lodash.defaults isn't going to change much, and doesn't matter too much as a dependency. Mostly just that if we can have consistent idioms in our code across codebases without using a dependency, then we should do that. Curious though what @thisandagain and @mewtaylor think of this.

I agree @mewtaylor I think we should keep react-formsy around until it causes us problems.

rschamp commented 7 years ago

We should also enable Greenkeeper.