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

Big refactoring #13

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Big refactoring #13

wants to merge 18 commits into from

Conversation

chvanikoff
Copy link

All the changes are described in details in the article: https://medium.com/@chvanikoff/lets-refactor-std-json-io-e444b6f2c580
This PR brings backward-incompatible changes so in case it will ever be merged, minor version of the library must be increased to 0.2.0.

@geolessel
Copy link
Collaborator

@hassox What are the chances of this getting merged in and released soon?

@geolessel
Copy link
Collaborator

@chvanikoff Why did you remove the __using__ functionality?

@chvanikoff
Copy link
Author

@geolessel because it's useless and is not actually a functionality, but requirement: you have to perform unnecessary initial setup (create module, add it to your app's supervision tree) and the only thing you get from it is an ability to call MyApp.MyIoServer instead of just StdJsonIo.

@hauntedhost
Copy link

This seems well thought out, would also love to see this merge. Also had problems with current version because of 64k chunk issue. Let me know if there is anything I can do to help get this accepted and merged. I can test it further on my own apps, load test it for perf issues, and/or add additional unit tests if felt needed.

@chvanikoff
Copy link
Author

@somlor you are welcome to test the PR in any ways you can think of, I would be very thankful for a feedback. If you know about any use-cases not yet covered by tests - let me know as well.

@hassox
Copy link
Owner

hassox commented May 23, 2017

@chvanikoff just reading the comments. The using module provided for this was to prevent global configuration and allow for multiple jobs apps to be mounted and rendered. I haven't been paying attention to the job side of this lib so not sure if that is still a thing that would need to be handled on this side

@hassox
Copy link
Owner

hassox commented May 23, 2017

The main use case I have in mind for such a multi app setup is when mounting another app as a library inside your main application (although that is not the only use case).
For example the (exq lib)[https://github.com/akira/exq] can run as a stand alone app, or be mounted in a hot app yet provides it's own front end. (I know they use ember but the pattern is the same)

@chvanikoff
Copy link
Author

@hassox hi, glad you joined the discussion.
About configuration - it's not a big deal to add StdJsonIo.configure/1 and don't start workers in case config is empty. So instead of defining a module you still will only have to write something like StdJsonIo.configure(script: "rect-stdio") in Application.start callback, for example. Similar to how poolboy works.
About different runners - never thought about it / used it in a such way, but it's also not that hard to add a support for this.

@hassox
Copy link
Owner

hassox commented May 23, 2017

I think it's the different runners that I'm concerned about. If support for that is removed then lib writers would not be able to use it (I believe) for fear of stepping on the main application. Is that assumption still correct or has the js side been updated to be able to handle it?

@hassox
Copy link
Owner

hassox commented May 23, 2017

Sorry hit send too quickly. I also imagine that ember and elm will eventually get a server side renderer and I'm wondering how that would be supported.

@chvanikoff
Copy link
Author

different runners can be implemented somehow like this:

# in runtime
configure(runner, config)
# or via config 
config :std_json_io,
  react: %{
    script: "react-stdio"
    ...
  },
  elm: %{
    script: "some-elm-renderer.js"
    ...
  }

and then any library can run configure("some_lib_react", %{script: "my-react-stdio"})

Sorry, not sure I understand what you mean by JS-side update.

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.

4 participants