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

no way to programmatically discover node:test #42785

Open
ljharb opened this issue Apr 19, 2022 · 24 comments · Fixed by #43396
Open

no way to programmatically discover node:test #42785

ljharb opened this issue Apr 19, 2022 · 24 comments · Fixed by #43396
Labels
module Issues and PRs related to the module subsystem.

Comments

@ljharb
Copy link
Member

ljharb commented Apr 19, 2022

Version

18.0.0

Platform

No response

Subsystem

No response

What steps will reproduce the bug?

require('module').builtinModules

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior?

Everything that's requireable that ships with node is listed (or, the listed items are requireable with node: added).

What do you see instead?

node:test is not listed.

Additional information

There needs to be a way to programmatically discover all builtin modules. require('module').builtinModules is supposed to be it.

This broke assumptions in my tests for https://npmjs.com/is-core-module - specifically, CIGTM should have failed prior to node 18 going out, because node:test wasn't part of is-core-module, but by making it effectively "secret", my tests didn't know about it.

@ljharb
Copy link
Member Author

ljharb commented Apr 19, 2022

cc @nodejs/modules @cjihrig @nodejs/test_runner

ljharb added a commit to inspect-js/is-core-module that referenced this issue Apr 19, 2022
ljharb added a commit to inspect-js/is-core-module that referenced this issue Apr 19, 2022
@aduh95
Copy link
Contributor

aduh95 commented Apr 19, 2022

import("node:test").then(()=>true, ()=>false)?

@ljharb
Copy link
Member Author

ljharb commented Apr 19, 2022

That's not synchronous, and requires that I hardcode the identifier in advance.

I should be able to write code that works for all future prefix-only core modules without ever needing to hardcode their names.

@jasnell
Copy link
Member

jasnell commented Apr 19, 2022

@cjihrig ... Looking through the code it appears that not listing node:test in builtinModules was intentional ... though it's not clear why. I agree with @ljharb that it likely should be there. Adding it, however, does break a handful of tests so I wanted to check to see what needs to be considered there first.

@aduh95
Copy link
Contributor

aduh95 commented Apr 19, 2022

@ljharb there's a workaround, but it requires to use --expose-internals, sharing in case that unblocks you:

'use strict';

const { internalBinding } = require('internal/test/binding');
const {
  moduleCategories: { canBeRequired },
} = internalBinding('native_module');

console.log(canBeRequired.has('test')); // true

@ljharb
Copy link
Member Author

ljharb commented Apr 19, 2022

@aduh95 thanks; good to know, but i don't think it suffices.

I think that the solutions here are either:

  1. add node:test to builtinModules
  2. make test requireable
  3. add a new list of prefix-only core modules

My preference is the second one, the first is the simplest, and the third imo would be more of an argument for the second one because of the user confusion it furthers.

@devsnek
Copy link
Member

devsnek commented Apr 19, 2022

I would prefer option 1, it seems acceptable as an opaque string.

@jasnell
Copy link
Member

jasnell commented Apr 19, 2022

Option 2 has already been settled by the TSC vote. That makes options 1 and 3 the viable paths forward here. My preference would be for option 1. As far as I can tell, that shouldn't actually break anyone except a couple of our tests.

@cjihrig
Copy link
Contributor

cjihrig commented Apr 19, 2022

I'm also in favor of option 1 (only because option 2 is off the table).

@aduh95
Copy link
Contributor

aduh95 commented Apr 19, 2022

An option 4 would be to freeze and expose the canBeRequired set. I agree that option 1 and 3 are also viable paths, and option 1 is probably the simplest (although it might still be a breaking change?)

@ljharb
Copy link
Member Author

ljharb commented Apr 19, 2022

I'm not sure why it would be - was it ever communicated that the API of this list is that nothing has a node prefix?

My tests were doing "builtinModule item", and "builtinModule item with a node prefix" - so i did have to change the logic to "only add the node prefix if it's not already there". That's a pretty minimal change tho, and arguably i shouldn't have hardcoded the assumption that things in that list don't have the prefix. So option 1 does seem like it'd be fine.

@ljharb
Copy link
Member Author

ljharb commented Apr 19, 2022

another oddity i noticed is that most all the builtin modules are available as globals in the repl - except for ones like module that are shadowed by the CJS module - and test is not available there. Should I file a separate issue for that? It seems like there'll be a bunch of unaccounted-for edge cases with the "prefix-only" approach the TSC vote unfortunately settled on.

@jasnell
Copy link
Member

jasnell commented Apr 19, 2022

The repl limitation is known already. It's not worth opening an issue, I think. Prefix only modules just won't be available as globals in the repl.

@ljharb
Copy link
Member Author

ljharb commented Apr 19, 2022

That's probably fine for the test runner, but I suspect it will be pretty limiting if future modules are added prefix-only. I guess I'll wait until that happens to bring it up again tho.

@ljharb
Copy link
Member Author

ljharb commented Jul 8, 2022

@aduh95 this issue is not fixed; isBuiltIn doesn’t do it because you can’t discover node:test with it. Please reopen.

@aduh95 aduh95 reopened this Jul 8, 2022
targos pushed a commit that referenced this issue Jul 12, 2022
PR-URL: #43396
Fixes: #42785
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@hemanth
Copy link
Contributor

hemanth commented Jul 15, 2022

@ljharb do we see a need for module. builtinPrefixOnly ? I can work on cooking that if we decide to have one.

@ljharb
Copy link
Member Author

ljharb commented Jul 15, 2022

@hemanth possibly, but that still wouldn't address this issue either. We likely need an API that provides an iterator over all of the names of builtin modules (fs, node:fs, node:test, etc), or, a Set instance containing them, or something similar.

@hemanth
Copy link
Contributor

hemanth commented Jul 15, 2022

@ljharb trying to recall on why we didn't ended up populating builtinModules will all of the prefix, non-prefix and prefix-only modules to begin with.

@ljharb
Copy link
Member Author

ljharb commented Jul 15, 2022

Probably because the assumption was that there'd never be a prefix-only module, but node:test broke that axiom :-(

@MoLow MoLow added the module Issues and PRs related to the module subsystem. label Jul 17, 2022
@hemanth
Copy link
Contributor

hemanth commented Jul 18, 2022

Hmm, need to verify if there are any side effects of populating builtinModules with all of it.

@hemanth
Copy link
Contributor

hemanth commented Jul 20, 2022

Module.builtinModules = ObjectFreeze([...allBuiltins]); // in loader.js

All of the module tests pass.

Should I do a PR if there are no other concerns? //cc @nodejs/modules

targos pushed a commit that referenced this issue Jul 31, 2022
PR-URL: #43396
Fixes: #42785
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@aduh95 aduh95 reopened this Sep 11, 2022
guangwong pushed a commit to noslate-project/node that referenced this issue Oct 10, 2022
PR-URL: nodejs/node#43396
Fixes: nodejs/node#42785
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@loynoir
Copy link

loynoir commented Apr 7, 2023

#47465

> nodeModule.isBuiltin('node:test')
true

After node: protocol introduced, and node:X and X is now kind of different, does it make sense to add .builtinModuleIds, and deprecate .builtinModules?

.builtinModuleIds

  • it is something like ["X", "node:X", "X/promises", "node:X/promises"]

  • in ESM, it refers to AST level import statement importId

  • in CJS, it refers to AST level require expression arg[0]

  • Compared with .builtinModules which use concept module, now a layer over raw AST concept, leads to different instinct between node dev and userland dev

sverweij added a commit to sverweij/dependency-cruiser that referenced this issue Jul 15, 2023
## Description

- ensures `node:test` is detected as a core module

## Motivation and Context

Currently `node:test` dependencies are classified as unable to resolve.
This is because `node:test` or `test` are not listed as `builtinModules`
in node's `module` module. From the discussion on
nodejs/node#42785 I understand this is never
going to happen either. Alternative is to use `module`'s `isBuiltin()`
function, but that is not available in the lowest version of nodejs
dependency-cruiser supports (16.14), so we have to work around that.

Additionally the `node:test` module can _only_ be required via the
`node:` protocol (`const { test} = require('test')`/ `import { test }
from 'test'` don't work). This means that with the current the way we
split the protocol from the module name, using `isBuiltin` won't work
either...

## How Has This Been Tested?

- [x] green ci
- [x] additional automated tests

## Types of changes

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Documentation only change
- [ ] Refactor (non-breaking change which fixes an issue without
changing functionality)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)

## Checklist

- [x] 📖

  - My change doesn't require a documentation update, or ...
  - it _does_ and I have updated it

- [x] ⚖️
- The contribution will be subject to [The MIT
license](https://github.com/sverweij/dependency-cruiser/blob/main/LICENSE),
and I'm OK with that.
  - The contribution is my own original work.
- I am ok with the stuff in
[**CONTRIBUTING.md**](https://github.com/sverweij/dependency-cruiser/blob/main/.github/CONTRIBUTING.md).
@ljharb
Copy link
Member Author

ljharb commented Jul 12, 2024

This issue also masks the existence of node:sea and node:sqlite.

@RedYetiDev
Copy link
Member

This issue also masks the existence of node:sea and node:sqlite.

And it's likely that more will follow. For my 2 cents, the most logical approach IMO is to populate the builtinModules list with all modules, each labeled with the node: prefix (since having node:sea alongside fs (not node:fs) wouldn't make sense IMO).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants