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

Node-compatible process module #10782

Closed
wants to merge 5 commits into from
Closed

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented Mar 19, 2019

JIRA: https://jira.appcelerator.org/browse/TIMOB-26571

Description:
This is an attempt to expand the global process object to be more compatible with Node when possible.

What's missing?

  • A number of the APIs are basically stubs and are therefore no-ops or expected to be unused
  • process.getegid()
  • process.geteuid()
  • process.getgid()
  • process.getgroups()
  • process.getuid()
  • process.hrtime([time])
  • process.hrtime.bigint()
  • process.initGroups()
  • process.kill()
  • process.mainModule
  • process.memoryUsage()
  • process.nextTick()
  • process.release
  • process.send(message[, sendHandle[, options]][, callback])
  • process.setegid(id)
  • process.seteuid(id)
  • process.setgid(id)
  • process.setgroups(groups)
  • process.setuid(id)
  • The process events aren't really fired outside of warning and uncaughtException. A number of them don't apply, but I think perhaps beforeExit may make sense to hook to our Ti.App event for close.

@build
Copy link
Contributor

build commented Mar 19, 2019

Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 3787 tests are passing.

Generated by 🚫 dangerJS against 1744d15

Copy link
Contributor

@janvennemann janvennemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor note regarding formatting of a doc block. Approving nonetheless, since the rest looks good!

@@ -0,0 +1,15 @@
/**
* @param {*} arg passed in argument value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove indentation to match other doc blocks.

process.noDeprecation = false;
process.pid = 0;
// FIXME: Should we try and adopt 'windowsphone'/'windowsstore' to 'win32'?
// FIXME: Should we try and adopt 'ipad'/'iphone' to 'darwin'? or 'ios'?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, yes, please! :)

@janvennemann
Copy link
Contributor

@sgtcoolguy i think it should somehow be configurable what node shims we load since there are some node modules out there that change behavior depending on availability of certain built-in node modules.

The popular axios http client for example tries to load the http module when global.process is available. Since we don't have that yet, requiring axios now breaks and our axios-adapter is not usable anymore. A common workaround would be to temporarily deleting process from the global before requiring the module and then re-attaching it.

We either need some documentation that describes these gotchas and how to workaround them or a way to disable the shims if a user does not need them anyway.

@sgtcoolguy
Copy link
Contributor Author

manually merged

@sgtcoolguy sgtcoolguy closed this Apr 15, 2019
@sgtcoolguy sgtcoolguy deleted the node-process branch April 15, 2019 15:34
@sgtcoolguy
Copy link
Contributor Author

@janvennemann Yeah I'm not sure how to handle "partial" node API support. To some degree we may be able to make use of browserify to shim the ones we haven't gotten to, but for example http from browserify will also fail.

We could use some sort of property/build flag to toggle the extensions altogether or maybe using some pattern matching or something?

It's difficult, because even something like trying to get our test suite to use the "out of the box" mocha with these shims keeps exposing more subtle API differences we need to fix. Some of which are much larger architectural issues (like how Android's require() is much more compatible with Node and iOS/Windows are more natively implemented and don't support say module.paths).

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

Successfully merging this pull request may close these issues.

None yet

3 participants