Skip to content

node10, optional electron support with example, updated build, buffer read/write support#24

Closed
d4tocchini wants to merge 10 commits intomogill:masterfrom
d4tocchini:master
Closed

node10, optional electron support with example, updated build, buffer read/write support#24
d4tocchini wants to merge 10 commits intomogill:masterfrom
d4tocchini:master

Conversation

@d4tocchini
Copy link
Copy Markdown

@d4tocchini d4tocchini commented Nov 12, 2018

  • updated make build flow
  • nodejs v10 support (nan not yet n-api)
  • builds native electron (v3.x.x v4.beta.x & v4.nightlies) add-on if electron detected up parent node_modules
  • tests updated & green for nodejs & electron
  • bit more idiomatic js directory & path conventions
  • made internal lib paths relative || computed instead of relying on parent/global module name resolution
  • updated npm deps
  • example ems file system in electron app (see Examples/emsfs/README.md)

msavva and others added 7 commits July 2, 2018 15:34
…ds native electron (v3.x.x v4.beta.x & v4.nightlies) add-ons if electron detected up parent node_modules, tests updated & green for nodejs & electron, bit more idiomatic js directory & path conventions, made internal lib paths relative instead of relying on parent/global module name resolution, updated npm deps
@d4tocchini d4tocchini changed the title node10 & optional electron support, added example, updated build config node10 & optional electron support with example, updated build config Nov 12, 2018
@mogill
Copy link
Copy Markdown
Owner

mogill commented Nov 13, 2018

Thank you, Dan and @msavva, for the PR. It forced me to notice that some people actually were using and maintaining their forks. This came as a complete surprise to me as the Node community has been trying to bury EMS since I told T.J. Fontaine about it in 2014, and I've only confirmed one person ever used EMS in production, and that was for a one-off project a few years ago.

I'm new to Electron so this PR was a bit overwhelming and I've had to spend some time learning the build/test/release process and how you refactored EMS to support Electron. I was saddened to see in addition to myself at least two other people struggled with PIP, and it's not clear 5bab154 gets PIP to work either.

It looks like the Node v10 problem with NAN required a three line patch, but refactoring for N-API will still be the long-term fix.

I'm looking forward to understanding the emsfs example and merging this once all the tests are passing again, hopefully I can get to it before Thanksgiving. Again, thanks for your interest and effort.

@d4tocchini
Copy link
Copy Markdown
Author

@mogill, which tests weren't passing for you? It looked like they were green on my end. On that note, i was pondering putting in a more common node.js spec reporter. Would be nice to see summarized test results with basic benchmarking. There's many to choose from; ava, tape, mocha, etc., and of course, it's trivial to roll your own without all the bells & whistles. Is there specific direction you'd want to go? Ava.li seems like a good fit. I'm hoping to get time to spec out buffer support and could start porting the js tests.

EMS seems a natural fit for electron apps. There's quite a bit of wahoo around IPC, how traditional server-clients fit into the app architecture, and sharing state between the main nodejs proc and browser windows that could be substantially improved by EMS, both in perf & ease-of-use. FYI, I'm well into the process of rolling out EMS in a sizable production electron app, will let you know when ready.

@d4tocchini
Copy link
Copy Markdown
Author

oh, if by tests failing you were referring to travis, it's probably the node version target. I only tested locally with node 10.11.0 & 10.12.0

@d4tocchini
Copy link
Copy Markdown
Author

For basic context on native node modules in electron:
https://electronjs.org/docs/tutorial/using-native-node-modules

on general perspective for this sort of thing:
https://electronjs.org/docs/tutorial/multithreading
https://code.visualstudio.com/blogs/2018/03/23/text-buffer-reimplementation (if you haven't already, checkout VSCode, shining example of an electron app that's rocketed to editor of choice for many, most javascripters & myself)

Electron's desktop app API sits atop forks of chromium. node & v8. Thus, brings a bucket of potential issues with native node modules, which explains why the electron node-gyp buid stuff is way more verbose than then nodejs equivalent.

@mogill
Copy link
Copy Markdown
Owner

mogill commented Nov 13, 2018

The Travis logs show two different types of failure:

Because both the build and the Py setup file changed I wasn't sure what the root cause was. Travis is executing make test if you want to replicate the failures from the command line.

Looks like I have more homework w/r/t Electron before I'm caught up.

…s but reads are still cast utf8 strings,need proper specs
…cpy at native<->nodejs boundaries, opens door to shared media/binary assets, prior API specs still green, need specs, temp hello world proof in Examples/emsfs
@d4tocchini
Copy link
Copy Markdown
Author

  • EMS_TYPE_BUFFER support

@d4tocchini d4tocchini changed the title node10 & optional electron support with example, updated build config node10 & optional electron support with example, updated build, buffer read/write support Nov 14, 2018
@d4tocchini d4tocchini changed the title node10 & optional electron support with example, updated build, buffer read/write support node10, optional electron support with example, updated build, buffer read/write support Nov 14, 2018
…odern const/let usage with some attention spent avoiding v8 deopts, ungnarled the deeper if/else trees, tried to humanize the top-level multi-branched arg handling, removed ref to no longer needed proxy polyfill, removed hanging log statements, removed adandoned/unhelpful util functions, etc
@mogill
Copy link
Copy Markdown
Owner

mogill commented Nov 26, 2018

Thanks for your continued efforts, Dan. I think this one PR has evolved to cover four different things:

  • The fix for the native/NAN build problem and Travis tests for Node v10
  • ES6 and Node style improvements to both EMS and the tests
  • Addition of the EMS buffer type
  • Electron support

Sadly, I haven't had the time to deal with the PR as a whole (buffer type needs unit tests, and I suspect the Python build is broken due to the Electron related changes), so I'd like to merge changes in grouped by purpose. If you could split this PR that would be helpful, otherwise I'll just keep falling further behind even though parts of this look fine.

@d4tocchini
Copy link
Copy Markdown
Author

@mogill, yes, totally agree. forgive the broad spectrum pushes here, i'm currently juggling this with an intense dev schedule and thus this is more of a raw stream than a proper PR. Partly, my hope was to engage you personally, to ensure I didn't veer too far from what you established and get something moving forward.

I've started the buffer specs and will give Python a proper pass for the build updates. Given updates in test coverage and all tests pass, any other feedback for me to consider? I'll close this and get to the atomizing the PRs

@d4tocchini d4tocchini closed this Nov 26, 2018
@mogill
Copy link
Copy Markdown
Owner

mogill commented Dec 2, 2018

I merged @msavva's NAN fixes for Node v10, dropped v0.10 support, and EMS is passing all tests again. I also added a notice to the README that a N-API port is not currently planned, I don't want event-stream to happen to me, either.

I'd like to merge in @d4tocchini's style and ES6 commits next, but a N-API port would probably be more valuable long-term. Perhaps both someday...

@mogill
Copy link
Copy Markdown
Owner

mogill commented Jan 2, 2019

The Node Foundation apparently has resources to maintain abandoned NAN packages, but they seem focused on benefiting from early volunteer support more than growing the Node ecosystem.

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.

3 participants