Skip to content

Conversation

@YannDanthu
Copy link

Summary

This PR adds dual CommonJS/ESM support to the Node.js bindings, allowing the package to be used with both require() and import statements.

Changes

  • ✅ Add ESM build output (gtfs-realtime.mjs, types.d.mts)
  • ✅ Update package.json with conditional exports for dual-mode support
  • ✅ Split buildProto script into modular cjs/esm build targets
  • ✅ Automate Long import fix to use protobufjs stub type
  • ✅ Automate default export syntax fix for ESM
  • ✅ Remove 'type: commonjs' field to allow proper dual-mode resolution
  • ✅ Document git submodule initialization requirement

Usage

The package now automatically serves the correct module format based on the importing environment:

ESM:

import { transit_realtime } from 'gtfs-realtime-bindings-transit';

CommonJS:

const GtfsRealtimeBindings = require('gtfs-realtime-bindings-transit');

Backward Compatibility

Full backward compatibility maintained with existing CommonJS users.

Test Plan

  • Test ESM imports in a module project
  • Test CommonJS requires in a traditional Node.js project
  • Verify TypeScript type definitions work for both formats

This commit adds dual CommonJS/ESM support to the Node.js bindings,
allowing the package to be used with both require() and import statements.

Changes:
- Add ESM build output (gtfs-realtime.mjs, types.d.mts)
- Update package.json with conditional exports for dual-mode support
- Split buildProto script into modular cjs/esm build targets
- Automate Long import fix to use protobufjs stub type
- Automate default export syntax fix for ESM
- Remove 'type: commonjs' field to allow proper dual-mode resolution
- Document git submodule initialization requirement

The package now automatically serves the correct module format based
on the importing environment while maintaining full backward compatibility
with existing CommonJS users.
Copy link

@magoni magoni left a comment

Choose a reason for hiding this comment

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

Looks fine to me, although I haven't tested the changes.

Do we need to manually increment the version?

Since I'm less familiar with this project I'll let another maintainer weigh in for the final review

@YannDanthu
Copy link
Author

Looks fine to me, although I haven't tested the changes.

Do we need to manually increment the version?

Since I'm less familiar with this project I'll let another maintainer weigh in for the final review

good point.
the nodes/UPDATING.md says yes.
and I guess after the PR merge we have to do a npm publish

Copy link

@cmonagle cmonagle left a comment

Choose a reason for hiding this comment

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

Didn't test, but LGTM.

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.

4 participants