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

Remove our use of default exports #945

Open
3 of 12 tasks
rowanmanning opened this issue Feb 21, 2024 · 0 comments
Open
3 of 12 tasks

Remove our use of default exports #945

rowanmanning opened this issue Feb 21, 2024 · 0 comments
Labels
breaking change Indicates that the feature will require a major version bump. enhancement New feature or request

Comments

@rowanmanning
Copy link
Member

rowanmanning commented Feb 21, 2024

Default exports cause a lot of issues with the package types, we should stop using them and use named exports across any Reliability Kit modules we expect to be imported (i.e. all except eslint-config). This would be a breaking change.

What problem does this feature solve?

Default exports cause many of the compatibility issues between CommonJS and ESM. They also require us to make adjustments to get the types to work properly. If we weren't using default exports then we'd sidestep some of the issues with TypeScript and esModuleInterop, effectively reducing the number of module systems we test against by two.

If we stop using default exports then it's also a lot easier to start writing our code as ESM and cross-compiling to CommonJS (something I've experimented with but got blocked by our default exports). Writing as ESM feels more future-proof.

Ideal solution

We move all modules to used named exports, which would require code changes in downstream apps like this:

- const registerCrashHandler = require('@dotcom-reliability-kit/crash-handler');
+ const { registerCrashHandler } = require('@dotcom-reliability-kit/crash-handler');

- import registerCrashHandler from '@dotcom-reliability-kit/crash-handler';
+ import { registerCrashHandler } from '@dotcom-reliability-kit/crash-handler';

Although it requires a code change, it's very find/replaceable and we could do it with codemods (something that the Developer Automation team have been looking into).

If we think it's worthwhile, we could also introduce named exports to the modules as aliases in the short-term so that teams can start migrating ahead of the breaking change.

Alternatives

We could continue to use default exports but we'll be making it harder to migrate to ESM and we'd be accepting continued maintenance of several module compatibility tests that we could otherwise remove.

We could also start doing this work earlier for our low-level modules, e.g. app-info, serialize-request, and serialize-error – these modules are used by apps less frequently and mostly used in our own higher-level modules.

Checklist

These are the packages that need migrating. Some of them will just be a case of removing the default alias:

  • eslint-config
  • fetch-error-handler
  • opentelemetry
  • app-info
  • crash-handler
  • errors
  • log-error
  • logger
  • middleware-log-errors
  • middleware-render-error-info
  • serialize-error
  • serialize-request
@rowanmanning rowanmanning added enhancement New feature or request breaking change Indicates that the feature will require a major version bump. labels Feb 21, 2024
@rowanmanning rowanmanning moved this from 📥 Inbox to 📝 Planned in Reliability Kit Roadmap Feb 21, 2024
@rowanmanning rowanmanning added this to the Major versions 2024-11 milestone Feb 21, 2024
@rowanmanning rowanmanning changed the title Remove our use of default exports. Remove our use of default exports Feb 21, 2024
rowanmanning added a commit that referenced this issue Jun 12, 2024
This is going to cause us lots of problems when we want to export new
methods, e.g. methods to send custom metrics which I'll be doing in a
later commit. Nobody is using the JavaScript API at the moment and so
this is a safe major version bump for most. It's also worth addressing
this while use is still low.

See #945.
rowanmanning added a commit that referenced this issue Jun 12, 2024
This is going to cause us lots of problems when we want to export new
methods, e.g. methods to send custom metrics which I'll be doing in a
later commit. Nobody is using the JavaScript API at the moment and so
this is a safe major version bump for most. It's also worth addressing
this while use is still low.

See #945.
rowanmanning added a commit that referenced this issue Jun 12, 2024
This is going to cause us lots of problems when we want to export new
methods, e.g. methods to send custom metrics which I'll be doing in a
later commit. Nobody is using the JavaScript API at the moment and so
this is a safe major version bump for most. It's also worth addressing
this while use is still low.

See #945.
rowanmanning added a commit that referenced this issue Jun 12, 2024
This is going to cause us lots of problems when we want to export new
methods, e.g. methods to send custom metrics which I'll be doing in a
later commit. Nobody is using the JavaScript API at the moment and so
this is a safe major version bump for most. It's also worth addressing
this while use is still low.

See #945.
rowanmanning added a commit that referenced this issue Jun 19, 2024
This is going to cause us lots of problems when we want to export new
methods, e.g. methods to send custom metrics which I'll be doing in a
later commit. Nobody is using the JavaScript API at the moment and so
this is a safe major version bump for most. It's also worth addressing
this while use is still low.

See #945.
rowanmanning added a commit that referenced this issue Jun 19, 2024
This is going to cause us lots of problems when we want to export new
methods, e.g. methods to send custom metrics which I'll be doing in a
later commit. Nobody is using the JavaScript API at the moment and so
this is a safe major version bump for most. It's also worth addressing
this while use is still low.

See #945.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Indicates that the feature will require a major version bump. enhancement New feature or request
Projects
Status: 📝 Planned
Development

No branches or pull requests

1 participant