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.

Sign up for GitHub

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

Jump to bottom

process: add process.features.typescript #54295

Merged
merged 2 commits into from
Oct 5, 2024
+80 −14
Merged

process: add process.features.typescript #54295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3317be7
process: add `process.features.typescript`
RedYetiDev Aug 10, 2024
74a8769
fixup! process: add `process.features.typescript`
RedYetiDev Sep 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions doc/api/process.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1976,6 +1976,19 @@ added: v0.5.3

A boolean value that is `true` if the current Node.js build includes support for SNI in TLS.

## `process.features.typescript`

anonrig marked this conversation as resolved.
Show resolved Hide resolved
<!-- YAML
added: REPLACEME
-->

> Stability: 1.0 - Early development

* {boolean|string}

A value that is `"strip"` if Node.js is run with `--experimental-strip-types`,
`"transform"` if Node.js is run with `--experimental-transform-types`, and `false` otherwise.
aduh95 marked this conversation as resolved.
Show resolved Hide resolved

## `process.features.uv`

<!-- YAML
Expand Down
23 changes: 23 additions & 0 deletions lib/internal/bootstrap/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,29 @@ ObjectDefineProperty(process, 'features', {
}

const { emitWarning, emitWarningSync } = require('internal/process/warning');
const { getOptionValue } = require('internal/options');

let kTypeStrippingMode = null;
// This must be a getter, as getOptionValue does not work
// before bootstrapping.
ObjectDefineProperty(process.features, 'typescript', {
RedYetiDev marked this conversation as resolved.
Show resolved Hide resolved
RedYetiDev marked this conversation as resolved.
Show resolved Hide resolved
RedYetiDev marked this conversation as resolved.
Show resolved Hide resolved
__proto__: null,
get() {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to emit experimental warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree, we should not discourage feature detection, that'd just nudge folks into either disabling warnings and/or find another way for detecting typescript support.

Also, I don't think this should be a getter, there's nothing dynamic about it.

Copy link
Member

Choose a reason for hiding this comment

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

If we don't emit experimental warning here, and mark this as experimental, how can we remove this API if the --experimental-typescript-* flags are removed?

Copy link
Contributor

@aduh95 aduh95 Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Anything documented as experimental can be deleted at any time – and folks should write their code in a way that it still works whether process.features.typescript is false and undefined (they have to if they want their code to work on older release lines anyway), so removing it is unlikely to cause breakage. (If it is not documented as experimental or graduated to stable, it would need to go through a full deprecation cycle, or stay there forever, with a value of false always)

I expect lots of folks to prefer not use an API that may emit a warning (because users don't like them), so they would have to find another way to detect "TS" support, so what's the point of adding this in the first place if we don't want folks to use it?

Copy link
Member

@marco-ippolito marco-ippolito Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with @aduh95 , if typescript is enabled, it will emit the warning regardless, if typescript is disabled, it returns false. Even if it does not emit the warning we can remove it. I dont see something that will be hard to remove since it returns a falsy value anyways. And having a warning in application that are not currently using ts feels like would impact adoption.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. I'll remove the emitted warning when I get a chance.

Copy link
Member

Choose a reason for hiding this comment

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

Please don't resolve a review that isn't addressed. @RedYetiDev.

I recommend looking into this once again @marco-ippolito @aduh95. If you insist, I'll remove my block but this is an experimental feature that we should emit experimental warning for (according to documentation)

Copy link
Member Author

Choose a reason for hiding this comment

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

My apologies, I assumed this was resolved given the opposing opinions

Copy link
Contributor

Choose a reason for hiding this comment

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

I insist, I’m -1 on landing this with a runtime warning. I’d recommend using stability 1 Experimental and not stability 1.0 nonsense if that’s where the point of contention lies.

Copy link
Member Author

Choose a reason for hiding this comment

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

I marked it with the same stability that type strip support is labeled under, but I can change it

if (kTypeStrippingMode === null) {
RedYetiDev marked this conversation as resolved.
Show resolved Hide resolved
if (getOptionValue('--experimental-transform-types')) {
kTypeStrippingMode = 'transform';
} else if (getOptionValue('--experimental-strip-types')) {
kTypeStrippingMode = 'strip';
} else {
kTypeStrippingMode = false;
}
}
return kTypeStrippingMode;
},
configurable: true,
enumerable: true,
});

process.emitWarning = emitWarning;
internalBinding('process_methods').setEmitWarningSync(emitWarningSync);

Expand Down
28 changes: 28 additions & 0 deletions test/es-module/test-typescript.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,34 @@ test('execute a TypeScript test mocking module', { skip: isWindows && process.ar
strictEqual(result.code, 0);
});

test('expect process.features.typescript to be \'strip\' when --experimental-strip-types', async () => {
const result = await spawnPromisified(process.execPath, [
'--no-warnings',
'--experimental-strip-types',
'-p', 'process.features.typescript',
]);

strictEqual(result.stderr, '');
strictEqual(result.stdout, 'strip\n');
strictEqual(result.code, 0);
});

test('expect process.features.typescript to be \'transform\' when --experimental-transform-types', async () => {
const result = await spawnPromisified(process.execPath, [
'--no-warnings',
'--experimental-transform-types',
'-p', 'process.features.typescript',
]);

strictEqual(result.stderr, '');
strictEqual(result.stdout, 'transform\n');
strictEqual(result.code, 0);
});

test('expect process.features.typescript to be false without type-stripping', async () => {
strictEqual(process.features.typescript, false);
});

test('execute a TypeScript file with union types', async () => {
const result = await spawnPromisified(process.execPath, [
'--experimental-strip-types',
Expand Down
30 changes: 16 additions & 14 deletions test/parallel/test-process-features.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,22 @@
require('../common');
const assert = require('assert');

const keys = new Set(Object.keys(process.features));
const actualKeys = new Set(Object.keys(process.features));
const expectedKeys = new Map([
['inspector', ['boolean']],
['debug', ['boolean']],
['uv', ['boolean']],
['ipv6', ['boolean']],
['tls_alpn', ['boolean']],
['tls_sni', ['boolean']],
['tls_ocsp', ['boolean']],
['tls', ['boolean']],
['cached_builtins', ['boolean']],
['typescript', ['boolean', 'string']],
]);

assert.deepStrictEqual(keys, new Set([
'inspector',
'debug',
'uv',
'ipv6',
'tls_alpn',
'tls_sni',
'tls_ocsp',
'tls',
'cached_builtins',
]));
assert.deepStrictEqual(actualKeys, new Set(expectedKeys.keys()));

for (const key of keys) {
assert.strictEqual(typeof process.features[key], 'boolean');
for (const [key, expected] of expectedKeys) {
assert.ok(expected.includes(typeof process.features[key]), `typeof process.features.${key} is not one of [${expected.join(', ')}]`);
}
Loading