Skip to content

Switch to ESM modules #6603

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

Closed
wants to merge 59 commits into from
Closed

Switch to ESM modules #6603

wants to merge 59 commits into from

Conversation

lotas
Copy link
Contributor

@lotas lotas commented Oct 6, 2023

Motivation

In node.js ESM ("type": "module") package can import both CommonJS packages, and ESM packages. However, commonjs package cannot import newer ESM packages.

As more and more dependencies switch to ESM-only distribution, making upgrades (especially security upgrades) become hard.

Taskcluster platform consists of multiple utility (library) packages, services themselves, and clients.

Library packages: db, api, app, config, iterate, loader, monitor, postgres, pulse, references, testing, validate

Services: auth, built-in-workers, github, hooks, index, notify, object, purge-cache, queue, secrets, web-server, worker-manager

Clients that are published: taskcluster-client, taskcluster-client-web.

It is not possible to only convert services packages to ESM, as they depend on library packages, which also consume 3rd-party dependencies.

Clients, however, do not depend on those, so could possibly remain in CommonJS format to avoid shipping both ESM/CJS builds.

taskcluster-client is a published standalone package, but uses unlisted dependencies for testing: taskcluster-lib-*, taskcluster-testing. Those are not listed in package dependencies, as those packages are not installed, but are internal.
This was refactored, and implicit dependencies were removed, and tests that depends on those were moved into separate client/client-test folder

importing

ESM requires that files would be imported with extensions.

// This worked fine in cjs
const { util } = require('./util')

// this wouldn't work anymore
import { util } from './util'

// this will
import { util } from './util.js'
// or if it is a folder
import { util } from './util/index.js'

For the same reason (I believe) running node services/auth/src/main server will not work, but node services/auth/src/main.js server will work

__dirname, __filename

Can now be used with the help of import.meta.url:

const __filename = new URL('', import.meta.url).pathname;
const __dirname = new URL('.', import.meta.url).pathname;

module.parent no longer available

Not possible to do:

if (!module.parent) {
  load.crashOnError(process.argv[2]);
}

This will work:

import { fileURLToPath } from "url";
if (process.argv[1] === fileURLToPath(import.meta.url)) {
  load.crashOnError(process.argv[2]);
}

Testing

Since ESM require exports to be statically analyzable, test helper functions are being rewritten:
instead of exports.xx and dynamic injections of helper functions, helper.js now exposes single helper = {} object that is being modified by utility functions like withDb(), withPulse()

Exports

Taskcluster internal libraries expose everything as named export and as default to allow mixed usage:

export * from "./upgrade.js";
import { upgrade, downgrade } from './upgrade.js';
export default { upgrade, downgrade };

Can be used both ways:

import { upgrade } from 'taskcluster-db';
upgrade()
// or
import db from 'taskcluster-db';
db.upgrade()

Import assertions

Node.js allows to specify import assertions since v17:

// load json module
import jsonSchemaDraft06 from 'ajv/lib/refs/json-schema-draft-06.json' assert { type: 'json' };

// equivalent of 
const jsonSchemaDraft06 = require('ajv/lib/refs/json-schema-draft-06.json');

However, it is still experimental and comes with:

(node:40419) ExperimentalWarning: Importing JSON modules is an experimental feature and might change at any time

Also eslint doesn't implement it yet, waiting for it to be analysed.

Alternatives:

const dataDir = new URL('./data/yml/', import.meta.url).pathname;
const loadJson = filename => JSON.parse(fs.readFileSync(path.join(dataDir, filename), 'utf8'));
// or
const schemaPath = new URL('../../../node_modules/ajv/lib/refs/json-schema-draft-06.json', import.meta.url).pathname;
const jsonSchemaDraft06 = JSON.parse(fs.readFileSync(schemaPath, 'utf-8'));

Files were migrated in dumb way npx cjs-to-es6 path/to/files and does not result in a working code, there are still many steps to follow:

TODO

  • db/
  • infrastructure/tooling
  • libraries/api
  • libraries/app
  • libraries/config
  • libraries/iterate
  • libraries/loader
  • libraries/monitor
  • libraries/postgres
  • libraries/pulse
  • libraries/references
  • libraries/testing
  • libraries/validate
  • services/auth
  • services/built-in-workers
  • services/github
  • services/hooks
  • services/index
  • services/notify
  • services/object
  • services/purge-cache
  • services/queue
  • services/secrets
  • services/web-server
  • services/worker-manager

Fixes #4260

@lotas lotas requested a review from a team as a code owner October 6, 2023 10:55
@lotas lotas requested review from petemoore and matt-boris and removed request for a team October 6, 2023 10:55
@lotas lotas changed the title WIP: Ecmascript module [skip ci] WIP: Ecmascript modules [skip ci] Oct 6, 2023
@lotas lotas force-pushed the chore/ecmascript-modules branch 2 times, most recently from 1f04210 to 2744031 Compare October 6, 2023 16:16
@lotas lotas marked this pull request as draft October 6, 2023 20:05
@lotas lotas force-pushed the chore/ecmascript-modules branch 3 times, most recently from 1569572 to 3d673b1 Compare October 12, 2023 09:59
@lotas lotas changed the title WIP: Ecmascript modules [skip ci] WIP: Ecmascript modules Oct 12, 2023
@lotas lotas force-pushed the chore/ecmascript-modules branch 4 times, most recently from aad6218 to 2bd04f8 Compare October 17, 2023 09:00
@lotas lotas changed the title WIP: Ecmascript modules Ecmascript modules Oct 17, 2023
@lotas lotas changed the title Ecmascript modules Switch to ESM modules Oct 17, 2023
@lotas lotas force-pushed the chore/ecmascript-modules branch from 2bd04f8 to 165573c Compare October 17, 2023 09:01
@lotas lotas marked this pull request as ready for review October 17, 2023 09:04
@lotas lotas force-pushed the chore/ecmascript-modules branch from 165573c to 7503cce Compare October 17, 2023 09:06
@@ -0,0 +1,6 @@
audience: developers
level: major
Copy link
Contributor Author

Choose a reason for hiding this comment

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

although clients are mostly untouched, and are still CJS, worth making a major

@@ -23,3 +26,17 @@ exports.withRestoredEnvVars = () => {
}
});
};

const ROOT_DIR = path.resolve(__dirname, '../../..');
const suiteName = () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is copied from lib/testing to avoid having to implicitly depend on package from outside.

message: 'Internal error of sorts',
});
});
// suite(testing.suiteName(), function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as this whole test depends on "internal" libraries, it shouldn't be here.
figuring out where to put it

@lotas lotas marked this pull request as draft October 17, 2023 09:44
@lotas lotas marked this pull request as ready for review October 17, 2023 12:17
@@ -0,0 +1,35 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was added to keep clients/client clean, and separate tests that used internal libraries from the client itself.

@lotas lotas force-pushed the chore/ecmascript-modules branch from 698de97 to 960d829 Compare October 18, 2023 12:44
lotas added 26 commits October 18, 2023 16:53
To reduce footprint of this refactoring let's keep helper as it was
before. Helper.js defines and exports a `helper` object the same way
that it was doing before, but instead of `exports` it is using `helper`
object.
This allows to tests remain untouched (besides imports)
Import type assertions are still experimental and they disappoint eslint
Reverting to loading files with fs.readFile instead

eslint/eslint#15305
node client was using implicit dependencies, which makes it hard to keep
CJS because those dependencies are ESM already.

Some of those dependencies can be easily rewritten, but `retry_test.js`
heavily relies on the application logic, so doesn't belong there.
Retry logic tests from node client relied on implicit internal
dependencies that broken CJS/ESM interop. clients/client remain CJS and
can be used in both CJS and ESM packages.

Therefore we  extract those tests into separate package
cleaned up test-coverage.js
@lotas lotas force-pushed the chore/ecmascript-modules branch from befe0a4 to 63cf97b Compare October 18, 2023 14:53
@lotas lotas mentioned this pull request Oct 18, 2023
25 tasks
@lotas
Copy link
Contributor Author

lotas commented Oct 19, 2023

closed in favor of #6627

@lotas lotas closed this Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to ECMAScript Modules
1 participant