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

Make winston more ESM friendly #2006

Merged
merged 1 commit into from
Jan 10, 2022
Merged

Make winston more ESM friendly #2006

merged 1 commit into from
Jan 10, 2022

Conversation

miguelcobain
Copy link
Contributor

@miguelcobain miguelcobain commented Jan 8, 2022

This PR should make winston friendlier to ESM module named exports.

Basically, with ESM modules, we can't do this:

import { warn, error } from 'winston';

and instead have to do this:

import winston from 'winston';
const { warn, error } = winston;

This is a great blog post that explains the issue: https://simonplend.com/node-js-now-supports-named-imports-from-commonjs-modules-but-what-does-that-mean/

@DABH
Copy link
Contributor

DABH commented Jan 10, 2022

@miguelcobain Thanks for your contribution. For non-ESM modules, with your changes, will we still be able to to e.g. import { warn, error } from 'winston';? Or is this going to be a breaking change in how people are support to import the library?

@miguelcobain
Copy link
Contributor Author

miguelcobain commented Jan 10, 2022

This should be a non-breaking change for anyone using regular commonjs modules.
I'm a bit confused by your example. non-ESM users right now can't use import { warn, error } from 'winston'; because that's invalid syntax on non-ESM world.

You can certainly use that import syntax if you use some kind of transpilation step (e.g Babel or Typescript), but ultimately, those tools transpile to regular commonJS require calls under the hood.

I think this change should be non-breaking. Essentially, we're just making it easier for cjs-module-lexer to detect the commonJS named exports, thus improving the esm interoperability. We're just removing some indirection in this PR.

Basically, instead of doing:

const winston = exports;
winston.version = 123;

we're doing the same in a more direct and equivalent way:

exports.version = 123;

My main question is if things like Object.defineProperty(exports, 'level', { will be properly detected under ESM modules. Not sure if cjs-module-lexer is smart enough to understand that "export". But in any case, CJS users should see no difference, and ESM users would see a major ergonomic improvement.

@DABH
Copy link
Contributor

DABH commented Jan 10, 2022

Fair enough. I'm okay with trying this, and we can always update if any other users have issues.

@DABH DABH merged commit 2927964 into winstonjs:master Jan 10, 2022
@DABH DABH mentioned this pull request Jan 10, 2022
kenglxn added a commit to navikt/kursoversikt that referenced this pull request Feb 1, 2022
i tillegg:
- fjernet --experimental-modules flagget siden Dockerfile nå kjører med node:16
- bumpet prom api og client pga deprecation warning:
```
(node:22932) [DEP0152] DeprecationWarning: Custom PerformanceEntry accessors are deprecated. Please use the detail property.
    at PerformanceObserver.<anonymous> (/Users/g161400/dev/git/ditt-nav-arbeidsgiver/server/node_modules/prom-client/lib/metrics/gc.js:42:38)
```

re:
- https://github.com/winstonjs/winston/releases/tag/v3.4.0
- winstonjs/winston#2006
kenglxn added a commit to navikt/arbeidsgiver-innsyn-aareg that referenced this pull request Feb 2, 2022
i tillegg:
- fjernet --experimental-modules flagget siden Dockerfile nå kjører med node:16
- bumpet prom api og client pga deprecation warning:
```
(node:22932) [DEP0152] DeprecationWarning: Custom PerformanceEntry accessors are deprecated. Please use the detail property.
    at PerformanceObserver.<anonymous> (.../node_modules/prom-client/lib/metrics/gc.js:42:38)
```

re:
- https://github.com/winstonjs/winston/releases/tag/v3.4.0
- winstonjs/winston#2006
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.

None yet

2 participants