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

Should we refactor to decouple logic and view? #16

Closed
Armienn opened this issue Nov 21, 2017 · 12 comments · Fixed by #48
Closed

Should we refactor to decouple logic and view? #16

Armienn opened this issue Nov 21, 2017 · 12 comments · Fixed by #48

Comments

@Armienn
Copy link
Collaborator

Armienn commented Nov 21, 2017

Right now there's some heavy coupling, which makes the code harder to reason about. Anyone up for an attempt at some restructuring? Any comments about stuff that should be taken into consideration, etc?

@palexcast
Copy link
Collaborator

I agree, we should also probably split up the javascript into logical entities of some kind?

@ryepup
Copy link
Contributor

ryepup commented Nov 22, 2017

https://github.com/demipixel/factorio-blueprint/ looks to have a nice object model for the logic side of things.

@Zeragamba
Copy link
Contributor

Aye, we should decouple this stuff

@Armienn
Copy link
Collaborator Author

Armienn commented Nov 22, 2017

At a quick glance, https://github.com/demipixel/factorio-blueprint/ might almost be the entire logic side we need. I'm not sure about the proper way to include other projects, though. Do anyone else have experience with that, or should we just play it by ear?

@ryepup
Copy link
Contributor

ryepup commented Nov 22, 2017

Most large frontend projects have a build process that handles this sort of thing. I have experience with it and could set it up, but it would make it harder for newer developers to start working on the project. Instead of "edit the files and refresh", contributors would need to install a bunch of tools on their computer and learn how to use them.

Long term this is worth it because it makes it easy to add tests, linters (as desired in #18), split code amongst multiple files, reduce the size of the final file for faster downloads, safely use ES6 language features without fear of IE, etc.

On the other hand, this seems like a really fun project, and it keeping it simple would let more people contribute.

We can use libraries and stave off the complexity of a build step by using the unpkg service, which would let us use libraries from <script> tags, but factorio-blueprint would need to make a few changes to their build process to get in there.

@demipixel
Copy link

Autotorio compiles it pretty effortlessly so I'd take a look at that.

As for "including it", I added a license to ease worries and I really want to encourage people to use it, just try not to sell factorio-blueprint itself or copy it and say it's your own without making any changes.

Easiest method will probably be a script that downloads/updates factorio-blueprint (and other dependencies you might want) and then compiles everything together in one file.

@lessthantrue
Copy link
Contributor

I'm not familiar with how github projects work, but it seems like this feature would be well suited for it so we know what kind of progress is being made. Could someone with write access look into that?

@Zeragamba
Copy link
Contributor

Github projects is very simliar to an agile board

@lessthantrue
Copy link
Contributor

The reason I ask is because I have some features in mind that should be waited on until we clean this up a bit, but I don't know who (if anyone) is working on this and what progress they've made. I also don't want to start this myself and have two different people solving the same problem two different ways if someone else is working on it.

Maybe start a new branch in this repository for this feature, so it can be worked on more collaboratively?

ryepup added a commit to ryepup/factorio_blueprint_editor that referenced this issue Nov 26, 2017
Used webpack and babel to build a version of factorio-blueprint that can run in the browser, exposed as `window.Blueprint`

In the long run probably want to check-in the webpack config and delete the copies of other people's libraries. For now leaving it out to remain  compatible with this pending PR camerongillette#21

refs camerongillette#16
@ryepup
Copy link
Contributor

ryepup commented Nov 27, 2017

Spent a little time looking at factorio-blueprint and the existing codebase, thinking about how to break this up to permit multiple views (#25).

Two options are jumping to mind:

  • we could pull in React/Vue/Angular; those all have very well-worn patterns for decoupling logic and view. I'm thinking that might be overkill, and it'd need to wait until after travis-ci is setup anyhow
  • implement a classic model-view-controller (MVC) or model-view-viewmodel (MVVM) architecture in vanilla js, with a few utility libraries

I like react, but it's got a hell of a learning curve, which makes me lean towards MVVM. The model is factorio-blueprint, the view-model is a fairly thin wrapper, and the views are more js functions that accept the view-model as input and return html (i.e. components).

Thoughts?

@lessthantrue
Copy link
Contributor

lessthantrue commented Nov 27, 2017

Considering the scale of the project, option 2 seems best. We don't need much extra for now, just a better way of organizing things.

If we plan on going full Model <=> something <=> view, it would be worth it to look into using some kind of template engine to make the conversion from the model easier.

@Zeragamba
Copy link
Contributor

+1 for option 2 as well. For now we're trying to avoid having people install Node and NPM to contribute to the project.

ryepup added a commit to ryepup/factorio_blueprint_editor that referenced this issue Nov 30, 2017
Have sidebar and preview implemented, tiles are next then I'll be able to delete a lot of code.

* breaking the big script.js into a view.js, responsible for the UI, and viewmodel.js, responsible for the data+logic
* lots of small functions
* starts to use factorio-blueprint as the model
* pull in TinyEmitter to get event-driven behavior between view and viewmodel

Still lots of bugs, but a decent evening's progress. Should be more straightforward work to clear out most of script.js, although they'll be some nasty merge conflicts to fix since this refactor touches basically everything.

refs camerongillette#16
refs camerongillette#42
ryepup added a commit to ryepup/factorio_blueprint_editor that referenced this issue Dec 1, 2017
* lots of small functions to build the grid and handle DOM events
* add a handful of custom events in the viewmodel to help keep in sync
  without a bunch of plumbing
* delete a lot of freshly dead code in script.js, including the big placeable array
* move some images into the placeables folder so they'll show up in the siderbar

view.js is probably too big; will be able to simplify a lot if ES6 is on the table (see camerongillette#45)

refs camerongillette#16
ryepup added a commit to ryepup/factorio_blueprint_editor that referenced this issue Dec 1, 2017
The loading process is pretty clumsy; jumping through hoops to get in/out directions for belts.

refs camerongillette#16
ryepup added a commit to ryepup/factorio_blueprint_editor that referenced this issue Dec 2, 2017
* fix some weird data coming from factorio-blueprint
* exclude curved rail; it's a special snowflake that will need a lot
  of work to handle properly
* fix broken/dead references in index.html
* prevent uncaught exceptions around mouse events
* eslint fixes
* version bump

refs camerongillette#16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants