Skip to content

[WIP] convert all packages to ESM #1947

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

bkeepers
Copy link
Contributor

This project currently uses a combination of CJS require/module.exports and ESM import/export. As part of my efforts to help finish porting to TypeScript, I keep running into issues where one module is exported using module.exports = … and another is using export function …, which means sometimes it can be loaded with import, and sometimes it has to use require.

I thought it might be easier (haha) better to first standardized on ESM by converting everything to import/export, so this branch is an attempt to remedy that. It has turned out to be way more work than I expected, and it's not done yet.

Summary of changes:

  1. Sets "type": "module" in all the package.json files, which tells node to treat all files as ESM modules, which among other things, blocks the use of CJS features like module.exports, require(), and __dirname.
  2. Replaces all require statements with import.
  3. Updates existing import statements to use full file paths instead of directories or extension-less imports, for example:
    - import foo from '.'
    + import foo from './index.js
    - import bar from './bar'
    + import bar from './bar.js'
  4. Replaces all module.exports = … with export const …/export function …/export default …
  5. Replaces __dirname with import.meta.dirname
  6. Updates mocha to latest version, and replaces ts-node with tsx because there are some bugs with using ts-node to load ESM files

There are a few things that I'm still working through:

  1. When import is called dynamically inside a function–and not statically at the top of a file–it is asynchronous. There is no way to make it synchronous like require() is. That means any dynamic loading (like json config files, or the configured security strategy), it is asynchronous. A lot of this setup currently happens in the constructor function of the Server class, and constructors cannot be async. The json files are easy to fix by calling JSON.parse(fs.readFileSync(…)), but there's not an immediately obviously solution to dynamically loading functionality like the security strategy. My current plan is to let this PR sit while I work on a new branch to refactor server initialization to allow asynchronous loading, and then rebase this.
  2. import.meta.dirname was added in NodeJS 20.11, so I need to add a helper function to make work with node 20.0-20.10. It's only a couple lines of code to use import.meta.url.
  3. There may be other unknown issues. The code currently builds with tsc -b and passes eslint, but the server fails to start and most of the tests are failing due to the first issue with asynchronous initialization.
  4. @signalk/server-admin-ui-dependencies is still a CJS module. All it does is verify dependencies on load, so I'm not sure there is any value in converting it to ESM, but I can look closer at it
  5. @signalk/server-api probably needs to be published as both ESM and CJS for CJS plugins that depend on it. It's easy to add support for both in the same package, it just requires another tsconfig and build step.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants