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

WIP: tools: add core files to @nodejs namespace #20922

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@cjihrig
Contributor

cjihrig commented May 24, 2018

This is probably not the ideal solution, but I wanted to at least get the ball moving after the little bit of 'worker' module naming drama.

This commit allows all of Node core's public modules to be accessed via the @nodejs namespace. The non-namespaced modules are not affected.

CJS:

'use strict';
const fs = require('@nodejs/fs');

console.log(fs.readFileSync(__filename, 'utf8'));

ESM:

'use strict';
import fs from '@nodejs/fs';

console.log(fs.readFileSync('/path/to/file.mjs', 'utf8'));

I haven't added tests or docs because I expect pushback. Open to other suggestions.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@devsnek

you should also add and not name.startswith('@nodejs') so we can add new modules that are only in the namespace but otherwise this is heavenly.

@mscdex mscdex added the in progress label May 24, 2018

@bnoordhuis

Needs some work but good idea.

@@ -310,6 +311,14 @@ def JS2C(source, target):
definitions.append(Render(value, lines))
initializers.append(INITIALIZER.format(key=key, value=value))
if (is_js == True and not name.startswith('_') and

This comment has been minimized.

@bnoordhuis

bnoordhuis May 24, 2018

Member

Just if (is_js and ... (and don't print.)

print(name)
key = 'nodejs_%s' % key
name = '@nodejs/%s' % name
definitions.append(Render(key, name))

This comment has been minimized.

@bnoordhuis

bnoordhuis May 24, 2018

Member

This creates duplicate source code strings. Less than ideal because they're pretty big. Try to reuse the ones created around line 310.

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented May 24, 2018

I like the idea.

@addaleax

This comment has been minimized.

Member

addaleax commented May 24, 2018

I’d prefer to do it in our loader rather than baking it into the binary? In particular, I’d like require('fs') === require('@nodejs/fs') etc. to be true (or at least I’d expect it as a user?).

(Also, I think this is more or less orthogonal to the worker issue – it’ll be easy enough to address that whether this change lands or not)

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 24, 2018

In particular, I’d like require('fs') === require('@nodejs/fs') etc. to be true (or at least I’d expect it as a user?).

I would definitely not expect that to be the case, particularly for core modules.

EDIT:

(Also, I think this is more or less orthogonal to the worker issue – it’ll be easy enough to address that whether this change lands or not)

Yes, the worker name issue can certainly be handled independently. This has been brought up and discussed a fair amount, so I decided to take a whirl at it.

@jasnell

This comment has been minimized.

Member

jasnell commented May 24, 2018

I would definitely not expect that to be the case

hmm... I'm with @addaleax on this.. from a user point of view, they should be strictly equal or we're going to run into a range of issues. APM's, for instance, would have to monkeypatch both in order to do their thing, and we can end up with global state management issues across modules.

For instance, try:

const b1 = require('buffer');
const b2 = require('@nodejs/buffer');

b1.Buffer(1)
b2.Buffer(1)

The deprecation warning ends up being emitted twice (once for each version of Buffer).

For the existing set of core modules, what I would expect is that the require('@nodejs/xyz') form is an alias for require('xyz'), returning the same instance of the module in either case. For new modules, there would only be the require('@nodejs/xyz') form. It's a bit more complicated but I think would be more of what users would expect, and avoids the kind of state issue illustrated above.

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented May 24, 2018

I still disagree on a couple things:

  • If I pass different strings to require(), I'd expect to get back different module objects. Relative or absolute file paths which refer to the same file are the exception. I wouldn't expect @foo/bar and bar to load the same modules from the node_modules directory.
  • The issue you mention about APMs should be trivially addressed by using a reusable function.
  • The issue regarding duplicate warnings is based solely on your assumption that they should be the same module object. If that is not your starting assumption, then it's not surprising that multiple modules can produce multiple warnings.

For the existing set of core modules, what I would expect is that the require('@nodejs/xyz') form is an alias for require('xyz'), returning the same instance of the module in either case. For new modules, there would only be the require('@nodejs/xyz') form. It's a bit more complicated but I think would be more of what users would expect, and avoids the kind of state issue illustrated above.

This type of inconsistency and unnecessary complexity is bad. This means expecting people to mentally track which modules are aliased and which are not.

We can also take advantage of a "fresh start" here to stop exposing underscored modules like lib/_http_agent.js, etc., which we probably can't do if all existing modules have a 1:1 mapping in the @nodejs namespace.

@addaleax

This comment has been minimized.

Member

addaleax commented May 24, 2018

Since we’re disagreeing about user expectations, I’ve started a poll on Twitter – seems like the best way to get people’s intuitions right now.

We can also take advantage of a "fresh start" here to stop exposing underscored modules like lib/_http_agent.js, etc., which we probably can't do if all existing modules have a 1:1 mapping in the @nodejs namespace.

I think that’s a great idea, but it’s a separate question, right?

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented May 24, 2018

  • If I pass different strings to require(), I'd expect to get back different module objects. Relative or absolute file paths which refer to the same file are the exception. I wouldn't expect @foo/bar and bar to load the same modules from the node_modules directory.

They could - one could just re-export the other.

@cjihrig cjihrig closed this May 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment