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

[sdk] change transpile target to ~~ES2020~~ ES2022 #5393

Open
3 tasks
pichlermarc opened this issue Jan 29, 2025 · 9 comments
Open
3 tasks

[sdk] change transpile target to ~~ES2020~~ ES2022 #5393

pichlermarc opened this issue Jan 29, 2025 · 9 comments
Assignees
Labels
needs:code-contribution This feature/bug is ready to implement target:next-major-release This PR targets the next major release (`next` branch) type:feature A feature with no sub-issues to address

Comments

@pichlermarc
Copy link
Member

pichlermarc commented Jan 29, 2025

Description

We've updated our guidelines to state that we only support ES2020 starting with 2.0. To avoid confusion when we release, we should actually transpile to ES2020 right away.

This issue is considered done when

  • SDK packages have been updated to use ES2020 ES2022
  • API (@opentelemetry/api) has been held back to the current state to avoid breaking in a minor version
  • semconv (@opentelemetry/semantic-conventions) is held back to current state to avoid breaking in a minor version.

Additional context:

Follows up on:

@pichlermarc pichlermarc added needs:code-contribution This feature/bug is ready to implement type:feature A feature with no sub-issues to address labels Jan 29, 2025
@pichlermarc pichlermarc added this to the OpenTelemetry SDK 2.0 milestone Jan 29, 2025
@pichlermarc pichlermarc added the target:next-major-release This PR targets the next major release (`next` branch) label Jan 29, 2025
@trentm
Copy link
Contributor

trentm commented Jan 30, 2025

From slack discussion: The semantic-conventions package is going to go back/remain compatible with node >=14. So, it will need to be handled similarly to the api package and not transpile to the latest.

IIUC, the current target (from tsconfig.base.json) is "target": "es2017",. Per https://node.green/#ES2017 that's a pretty safe base going back to Node.js v8 (api's min version). I don't think there is a particular reason to go beyond ES2017 for the semantic-conventions package.

@chancancode
Copy link
Contributor

chancancode commented Jan 31, 2025

As requested by @pichlermarc, moving my discussion here.

I was suggest that we want to at least consider ES2022 for SDK 2.x, because it supports top-level await (TLA) – it's supported since Node 14 and in the included in browsers baseline (widely available) for quite some time (but consumers are probably using a bundler anyway, which has also supported TLA for a long time). It would be quite handy to have, and in some cases, probably necessary to migrate form top-level require statements.

As a concrete example, #5298 was looking at a module like this:

import * as process from 'process';

let getMachineId: () => Promise<string>;

switch (process.platform) {
  case 'darwin':
    ({ getMachineId } = require('./getMachineId-darwin'));
    break;
  case 'linux':
    ({ getMachineId } = require('./getMachineId-linux'));
    break;
  case 'freebsd':
    ({ getMachineId } = require('./getMachineId-bsd'));
    break;
  case 'win32':
    ({ getMachineId } = require('./getMachineId-win'));
    break;
  default:
    ({ getMachineId } = require('./getMachineId-unsupported'));
}

export { getMachineId };

This is problematic as, the use of require means it's coupling CJS features/semantics to ESM syntax, and it's not possible to emit this as a proper ESM. This prevents us from publishing in ESM (alongside CJS), which IMO is an important option we should definitely leave open.

We can replace the require() with import(), but the problem is import() is async. With TLA that can be done:

import * as process from 'process';

let getMachineId: () => Promise<string>;

switch (process.platform) {
  case 'darwin':
    ({ getMachineId } = await import('./getMachineId-darwin.js'));
    break;
  case 'linux':
    ({ getMachineId } = await import('./getMachineId-linux.js'));
    break;
  case 'freebsd':
    ({ getMachineId } = await import('./getMachineId-bsd.js'));
    break;
  case 'win32':
    ({ getMachineId } = await import('./getMachineId-win.js'));
    break;
  default:
    ({ getMachineId } = await import('./getMachineId-unsupported.js'));
}

export { getMachineId };

Without TLA, we are kind of stuck. In this particular instance, since the function happens to be async, we can work with it:

import * as process from 'process';

let _getMachineId: () => Promise<string> | null = null;

export async function getMachineId(): Promise<string> {
  if (_getMachineId === null) {
    switch (process.platform) {
      case 'darwin':
        _getMachineId = await import('./getMachineId-darwin.js')).getMachineId;
        break;
      case 'linux':
        _getMachineId = await import('./getMachineId-linux.js')).getMachineId;
        break;
      case 'freebsd':
        _getMachineId = await import('./getMachineId-bsd.js')).getMachineId;
        break;
      case 'win32':
        _getMachineId = await import('./getMachineId-win.js')).getMachineId;
        break;
      default:
        _getMachineId = await import('./getMachineId-unsupported.js')).getMachineId;
    }
  }

  return _getMachineId();
}


export { getMachineId };

This changes the timing and semantics slightly, perhaps that's acceptable in this case, but I can see other places where this would be an important tool to have.

In general, wherever we are using require() in module scope today, if we want to switch to proper ESM, we will probably need TLA. Category of cases I can think of:

  • Trying to import an optional dependency
  • Trying to import a different file/dependency depending on the platform (web vs node, OS, node version, etc)
  • Conditionally import a polyfill as needed

Sometimes we may be able to get away with pushing the async operation deeper down/to be more lazy, but a lot of times it forces the rest of the code in that file/module to work around that, whereas TLA lets you deal with it upfront and write the rest of the code the same way as if it was a synchronous require().

There are some other nice-to-have language and library features (I think "target" and "lib" are not coupled), but I think TLA is the most important to discuss here, since it cannot be easily transpiled/down-leveled/polyfilled.

Just wanted to highlight that and make sure it was considered.

@xiaoxiangmoe
Copy link

don't use TLA, it may break require es module in cjs

@dyladan
Copy link
Member

dyladan commented Feb 5, 2025

@xiaoxiangmoe can you expand on that or provide an example? We're talking about the minimum supported syntax here, and IIUC we'd be talking about requiring es2022 as a minimum version supported by the runtime, which should allow us to use TLA. I guess you're advocating for keeping our minimum version lower.

According to node.green ES2022 support is 97% complete in all versions of node >=16.13.2. The only non-supported feature appears to be a regex related feature. Our support statement lists node 18 as the minimum supported runtime, however node 18 still supports CJS which does not support features like TLA, so even if a user is on a supported runtime they may be using a CJS application which would not be able to load OpenTelemetry if we decide to use TLA.

To me it seems to boil down to this:

  1. We cannot publish standards-compliant ESM while using require.
  2. We cannot use top-level await while retaining support for CJS applications.
  3. We cannot abandon use of require without using top-level await.

This means we have to make a choice between supporting existing CJS users and supporting ESM users. There are also middle-ground options which would be to separately publish es2022 packages for ESM users which use top-level await.

Let's talk about this at the SIG meeting tomorrow

@chancancode
Copy link
Contributor

chancancode commented Feb 5, 2025

My speculation is that they are pointing out/referring to the fact that:

  • Today, a CJS module/file can use the require() syntax to load a ESM module...
    • ...but only if the ESM module is fully synchronous, or else the require() would throw

I don't necessarily disagree that under today's convoluted Node inter-op rules, whether a ESM module is asynchronous is a visible feature to CJS consumers (but not to ESM consumers), so...

  1. if we are publishing only ESM, and
  2. we expect CJS consumers to consume those ESM modules, then
  3. adding TLA to a module changes its semantics and is potentially a breaking change, and so, in general...
  4. depending on where we are at with publishing, adding TLA to a module requires some careful considerations.

However, consider:

  1. import() is available to CJS
  2. My understanding is we are currently publishing only CJS, and will go into the 2.0 series with that on the current trajectory. So I don't expect that we will (or can?) switch to ESM-only in 2.x. As long as we are still publishing equivalent CJS (which can't have TLA) alongside ESM, then it should be fine to use TLA in the ESMs?
  3. The inter-op story could see more changes during the 2.x series, or CJS usage may die out in that time.

I don't disagree that the inter-op stuff needs to be tread carefully, but we can figure out those details when the time comes.

At this point, given the limited timeframe we have for the research and analysis, I think the main thing to focus on is whether there is a very good reason we want to stay at ES2020 (is someone asking for it?).

I get that from a usage/support perspective, supporting an older format = more conservative, thus seems like a better default. But on the maintainability side, ES2022 gives us some additional tools that could become important at some point during 2.x as we navigate the modules transition, so IMO it is more conservative to have them in case we need them – we don't have to use them today.

@xiaoxiangmoe
Copy link

xiaoxiangmoe commented Feb 5, 2025

I support raising it to es2022 or esnext, but we should ban TLA for node's cjs compatibility, as @chancancode 's statement.

@dyladan
Copy link
Member

dyladan commented Feb 5, 2025

Decision from SIG meeting today: we will change to ES2022 but will disallow TLA in linting rules in order to avoid breaking existing CJS users. Users which need to target a lower version of ECMAScript will need to transpile code to a lower version.

@dyladan dyladan changed the title [sdk] change transpile target to ES2020 [sdk] change transpile target to ~~ES2020~~ ES2022 Feb 5, 2025
@david-luna
Copy link
Contributor

I will allocate some time to work on it :)

@david-luna david-luna self-assigned this Feb 10, 2025
@david-luna
Copy link
Contributor

IIUC we now should compile with 2 different targets:

  • es2017 for api and semantic conventions package
  • es2022 for the rest

Then I guess we should review the build/release process since the compilation task for that workflow is using npm run compile script at the root of the project. This script just takes the root tsconfig.json file and references all the projects. I see 2 possible options:

  • remove references in tsconfig.json and have a separate script to compile api and semantic conventions
  • use lerna to run npm run compile in each pacakge separately

I think tsconfig references was made to speed up the compile process so I'll try 1st approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:code-contribution This feature/bug is ready to implement target:next-major-release This PR targets the next major release (`next` branch) type:feature A feature with no sub-issues to address
Projects
None yet
Development

No branches or pull requests

6 participants