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

Suggestions before our first release #2

Open
yorkie opened this issue Nov 19, 2019 · 9 comments
Open

Suggestions before our first release #2

yorkie opened this issue Nov 19, 2019 · 9 comments
Labels
enhancement New feature or request

Comments

@yorkie
Copy link
Member

yorkie commented Nov 19, 2019

At the codebase, renaming the below macros and functions are recommended:

  • RTNODE_FUNCTION should be DEFINE_FUNCTION.
  • rename the prefix rtnode to js, like js_object_to_string, js_object_set_method and js_object_set_property.
@yorkie yorkie changed the title Suggestion to refine the source names before our first release Suggestions before our first release Nov 19, 2019
@yorkie
Copy link
Member Author

yorkie commented Nov 19, 2019

Another points are:

@yorkie
Copy link
Member Author

yorkie commented Nov 19, 2019

And we should follow this spec https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html instead of Node.js Timers, thus setImmediate() is unnecessary working on this project.

BTW, browser doesn't own the process object at https://github.com/yodaos-project/rt-node/blob/master/src/js/global.js#L6, to remove the process module, we have the following tasks:

  • remove nextTick()
  • remove memoryUsage()

These functions should be delivered from ecosystem or user-land.

@yorkie
Copy link
Member Author

yorkie commented Nov 19, 2019

And we should follow this spec https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html instead of Node.js Timers, thus setImmediate() is unnecessary working on this project.

Sorry about that @lolBig, setImmediate() has its Web edition: https://developer.mozilla.org/en-US/docs/Web/API/Window/setImmediate, and a corresponding clearImmediate() should be here: https://developer.mozilla.org/en-US/docs/Web/API/Window/clearImmediate.

@yorkie yorkie added the enhancement New feature or request label Nov 19, 2019
@qile222
Copy link
Contributor

qile222 commented Nov 20, 2019

Here is what i think we have to do before first release, some are consistent with you @yorkie :

  1. currently all js files are packaged in ./src/rtnode-snapshots.c, so we should provide an entry for user code. as you said, remove the src/js/app.js and load it from user side
  2. module system only support load js which packaged in a c file, other ways like load from disk should be supported
  3. resolve dependences between modules, if js from disk is supported, the module system should be able to resolve module from absolute or relative path
  4. support native addon from user side

Other points like code style is not the highest priority, we can adjust it later.

Since rtev is not the same as uv, the definition and use of the API is also different, so use different names in order to avoid confusing them

@yorkie
Copy link
Member Author

yorkie commented Nov 20, 2019

Other points like code style is not the highest priority, we can adjust it later.

I'm -1 on this, coding style is the higher priority for upcoming contributor, we should prepare them before this release.

Since rtev is not the same as uv, the definition and use of the API is also different, so use different names in order to avoid confusing them.

The rtev looks too long, make another short name looks good to me.

@qile222
Copy link
Contributor

qile222 commented Nov 20, 2019

Other points like code style is not the highest priority, we can adjust it later.

I'm -1 on this, coding style is the higher priority for upcoming contributor, we should prepare them before this release.

Since rtev is not the same as uv, the definition and use of the API is also different, so use different names in order to avoid confusing them.

The rtev looks too long, make another short name looks good to me.

Done 🚚

@yorkie
Copy link
Member Author

yorkie commented Nov 21, 2019

How about removing the app.js and process.js?

@qile222
Copy link
Contributor

qile222 commented Nov 21, 2019

i will remove app.js after added user js code entry.
i think we should provide some APIs like memory usage for debugger, now they are in process.js, we can rename process.js to another

@yorkie
Copy link
Member Author

yorkie commented Nov 21, 2019

i think we should provide some APIs like memory usage for debugger, now they are in process.js, we can rename process.js to another

Can we provide those at the other place like another npm package?

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

No branches or pull requests

2 participants