Modularize Javascript Files

[sulcata]

Modularize the files in app/assets/javascript. It'd probably be best to incorporate Browserify or something.

[sulcata]

I still stand by this even without linting. Concatenating files is messy and we have certain Pokemon database files being included multiple times.

[coyotte508]

using something like browserify, as long as it's automated and easily usable, does sound like a good idea. Not sure why the issue is closed.

[sulcata]

I closed the eslint issue, not this one. Unless someone else closed this one. There are other options like Webpack or RequireJS we could use, I'll try looking into which will work better.

[coyotte508]

@sulcata now that the client can be run locally, with electron, browserify sounds an even better idea

[sulcata]

If we're sticking with CommonJS imports we should go with browserify. If we decide to switch to using ES6 imports we might want to consider webpack v2 (not stable yet) since it has some fancy tree shaking features to remove unused code that uglify can't remove.

edit: just realized we're not actually using CommonJS outside of node since we're concating lol

[coyotte508]

The thing is, currently, I could use require() if I want and the javascript in the electron app would work fine, but the web one would need browserify.

If, by using import, it would still work fine inside the electron app without any change or processing, then why not! I'm all for using recent settings.

[sulcata]

It should work just fine in the electron app provided we bundle the scripts. I'm not sure whether or not we want to bundle all the scripts. If we don't bundle the scripts for electron then we'd need to translate the imports to CommonJS during the build with babel since node/V8 doesn't support es6 imports yet (which isn't hard to set up).

If we're going the webpack route we might want to even consider ditching grunt since all our grunt tasks can be done with just webpack it seems.

[coyotte508]

https://medium.com/@housecor/browserify-vs-webpack-b3d7ca08a0a9#.xl97th5df

Let's go with webpack?

[coyotte508]

I'm going to get started on it. And reindent everything to 2 spaces because there'll be a lot of major changes.

[sulcata]

Well, in the time it took me to make my last post it looks like Webpack 2 finally released (after at least a year i think lol). Imo we should be going with the import/export syntax in the webclient files since it can be better optimized.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import

I've been working with Webpack 2 quite a bit over the past day for my calc so I'll see what I can add.

[sulcata]

I'll start working on switching to import/export unless you have any objections.

[coyotte508]

Go ahead! Keep in mind that what I've done is only partial and a lot of things are broken for now in the webpack branch.

[sulcata]

Yeah, I thought the bundle size looked a little small xd

It might be a good idea to reorganize certain things. Class definitions are sort of split over multiple files right now.

[coyotte508]

Well, Poke and BattleData are, none others as far as I know.

For BattleData, I'd rather they stay split. Maybe require commandhandling from battledata.js (and only from there) to make it complete?

For Poke, it's actually model separated from view. Maybe rename the one in teambuilder.js something else (like TeambuilderPoke) and make it inherit the other one?

I agree that some things can be reorganized, even split into further files (for example all the stuff in PokeInfo could have one file each in a "pokeinfo" folder, or some functions in frontend.js could be made to belong to their own module, or all the jquery loading at the end of some files could be grouped and done in another file), but I'm not sure about merging files.

[sulcata]

Sounds good.

I've almost got a good chunk of the source imports sorted. One of the larger issues will be getting pokedex linked properly since iirc it's not in any sort of module format.

[sulcata]

battledata is an easy fix with Object.assign for now. We might want to refactor a lot of this into the class syntax. It's much nicer looking and easier to maintain compared to actually messing with prototypes.

[sulcata]

I tried looking into setting up the HTML webpack plugin, but it looks like we're rendering the .ejs templates on the fly. If we're able to render them in advance, then we could have a <script> tag automatically inserted to reference our bundle. It'd also make code splitting much easier.

[coyotte508]

There's an ejs loader: https://github.com/okonet/ejs-loader

The thing is, the app behaves differently whether or not it's inside electron. For example, it remembers passwords when under electron, it's also a goal to add the registry, as well as offline teambuilder. The password thing will eventually possibly be changed, when I have a better grasp of Electron (currently a server could serve html to get back all password stored, not only the one of the current server. Goal is to use custom urls for each server, keep access to data like teams maybe by getting them from a local API, but save password in each domain's local storage). So I'm not sure converting ejs to html is currently possible, as something different is rendered in index.ejs (serverConfig = <%- JSON.stringify(config) %>;) depending if the app is in electron or not.

Also replay.ejs (see latest commit in master branch) is served differently depending on the battle id (though it probably could be gotten from the url with javascript).

I 100% agree with classes and class syntax.

If the current webpack branch works after tests, then we can put it to the main branch and then switch to class syntax whenever we have the time (maybe use this opportunity to overhaul indentation to 2 spaces if adding the class syntax would change the indentation level anyway)

One thing is that it seems every function needs to be defined inside the class{} bracket, so we'd need to find a solution for BattleData. (or maybe delay turning that specific function into a class)

[sulcata]

The class syntax has extends. We could also just modify the prototype after we create the class. The only additional restriction that es6 classes have is that you can't call the constructor without new.

If we're going to go as far as changing indentation levels, do you want to adopt a full on style/linter? xo doesn't need configuration and comes with 2-spaces by default. It can even fix certain issues (like indentation) automatically.

[sulcata]

It'd probably also help with finding missing imports by flagging undefined variables and incorrectly written imports.

[coyotte508]

I'm ok with some things like indentation and required semi-colon, i'm not ok with other things, like forcing one kind of quote, or rules about spaces between operators and expressions. I like to write if (a == b), I don't want an error because there's no space between ( and a, or because there's a space between a and ==.

And, that is as long as switching to class syntax would effectively ruin the indentation (I think it would, as functions need to be inside classes now). And both changes would have to be done at the same time (not overhauling once because of indentation, and then once again because of classes).

For now I'm trying to fix errors

[sulcata]

Anything you don't like you can disable (i.e. quotes and == checking). I don't think it has any rule for putting a space after the parenthesis.

[coyotte508]

Maybe we can work together on this? Enable indentation, undefined variables, semi-colons. Enable also other things that don't require the current code to be changed, for the things that do, see together what else should be enabled? (or not disabled)

Or maybe just enable the first 3 and the rest later.

[sulcata]

If we're enabling/disabling each rule then we should go with eslint to pick and choose. Maybe we could discuss on discord?

[coyotte508]

Sure, I'm there (always there kind of)

[coyotte508]

I've added teambuilder.js, with its own bundle.

The idea is that it loads heavy pokedex files (not yet done) such as list of moves per pokemon, and I want to load it later than the page, as it's not immediately needed.

Is there any way to make it share the modules of frontend.js, without duplicating everything?

Also jquery.min.js is included in the index, at first, and other scripts are loaded at the end. Is there any way to use that as jQuery, and have webpack/eslint assume that a jQuery and $ are globally defined?

[coyotte508]

Ok, shared dependencies between teambuilder.js and frontend.js, though i'm not sure what to do when there's more than 2 modules sharing dependencies.

The not-loading jquery (as it's already loaded) is not done.

[sulcata]

Webpack has code splitting. There are different ways to do so depending on the specific issue at hand though.
https://webpack.js.org/guides/code-splitting/

You can also define externals, which essentially lets you use globals as imports.
https://webpack.js.org/configuration/externals/#externals

[coyotte508]

Lost almost 300kB in the main js file thanks to externals!

I'm also thinking of maybe loading things like individual pokemon movepools on demand (using an API, like for smogon sets), to reduce teambuilder size. And dynamically loading would be awesome too. Still a lot of stuff to do.

[coyotte508]

I realize it was maybe premature to use import when V8 engine doesn't support it yet. It would have been useful to have PokeInfo or pokedex serverside, or even in the repl interpreter. Not gonna revert everything though, it'd be such a waste. Just hope they release a version (and node adapts to it) soon!

[sulcata]

Node might be a little slow in adopting it. I can set up babel to make it work serverside if you'd like.

You can do dynamic loading with require.ensure until eslint supports parsing the dynamic import feature (right now import(...) will throw an error even though webpack supports it).