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

Support named exports in the ESM mode #60

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

prantlf
Copy link

@prantlf prantlf commented Feb 25, 2024

Proposal

Allow importing single exported named functions:

import { bold, green } from "picocolors"

Problem

If named exports are used today, the following error will be thrown:

> rollup -c --environment NODE_ENV:production

[!] SyntaxError: Named export 'bold' not found. The requested module 'picocolors' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'picocolors';
const { bold, green } = pkg;

file:///Users/prantlf/Sources/github/d3/rollup.config.js:2
import { bold, green } from 'picocolors';
         ^^^^
    at ModuleJob._instantiate (node:internal/modules/esm/module_job:132:21)

Changes

Expose an ESM module with named exports: picocolors.mjs. Generate its content from picololors.js by build.mjs. Test that the exports can be imported by test/esm.mjs.

Also upgrade development dependencies. Keep the same major versions of chalk and clean-publish not to break the legacy Node.js tests.

Also include tests with Node.js 20 and move versions older than 18 to the legacy group. Upgrade GitHub actions for modern Node.js.

Also fixes #50.
Also fixes #59.

package.json Outdated
"sideEffects": false,
"description": "The tiniest and the fastest library for terminal output formatting with ANSI colors",
"scripts": {
"build": "node build-esm.mjs && tsc --module esnext tests/types.mts",
"test": "node tests/test.js"

Choose a reason for hiding this comment

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

Would it not be more logical to put the compiling of the types tests during the test script? As it is a way to tests that types are correct.

Also maybe adding --noEmit option to not produce any compiled files of the test, no ?

Copy link
Author

Choose a reason for hiding this comment

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

Would it not be more logical to put the compiling of the types tests during the test script? As it is a way to tests that types are correct.

Yes, it's a good idea. No need to build the tests with the shipping code. And when the tests are about to run, let's ensure that they're built. I made the change in c4c4789.

Copy link
Author

Choose a reason for hiding this comment

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

Also maybe adding --noEmit option to not produce any compiled files of the test, no ?

Is it possible to just execute the .mts file without compiling it to .mjs first? I wanted to execute the test too, not only to check the compilability.

@alexeyraspopov
Copy link
Owner

These are very good changes, but we're not yet dropping these much of old LTS Node.js versions.

On the subject, I honestly can hardly understand the reason you'd want to import every single color separately. It just puzzles me how this can be convenient.

With default export I don't need to go back and forth updating import (even if auto import enabled in an editor of choice). This feels like an unnecessary mental overhead.

@braebo
Copy link

braebo commented May 28, 2024

This feels like an unnecessary mental overhead.

Pressing the tab key imposes negligible mental overhead. I use named imports habitually by default, so it feels wrong when I'm forced to import an entire module even though I'm only using one or two functions from it.

@alexeyraspopov
Copy link
Owner

I'm forced to import an entire module even though I'm only using one or two functions from it.

This is Node.js, there is no tree-shaking, both import some_default and import { methodA, thingB } do import an entire module regardless of what you end up using.

Allow importing single exported named functions:

    import { bold, green } from "picocolors"

Expose an ESM module with named exports: `picocolors.mjs`. Generate its
content from `picololors.js` by `build.mjs`. Test that the exports can
be imported by `test/esm.mjs`.

Also upgrade development dependencies. Keep the same major versions
of `chalk` and `clean-publish` not to break the legacy Node.js tests.

Also include tests with Node.js 20 and move versions older than 18
to the legacy group. Upgrade GitHub actions for modern Node.js.
Export single named exports from `picocolors.d.ts`. Test
the functionality by `test/types.mts`.

Include both `*.ts` files in the package.

Include the test in the GitHub workflow.
Do not compile the tests together with the shipping code.
It's not needed at that time.

Use `npm test` to run all tests.

Move the compilation to the `pretest` script to execute it
during `npm test`. The typescript tests can be just compiled
by running `npm pretest`. All tests can be just run by `npm test`.
Single test can be run by `node/tests/<filename>`.
@prantlf
Copy link
Author

prantlf commented Jan 26, 2025

These are very good changes, but we're not yet dropping these much of old LTS Node.js versions.

Thank you! The old Node.js versions can continue working. But their test coverage will likely shrink. You'll want to keep up with the rest of the ecosystem, which is enhanced and bug-fixed only in their recent releases.

I suggest providing excellent support for the recent LTS versions and spend reasonable effort on not breaking the old Node.js versions. For example, the current build pipeline runs 3 tests on modern Node.js versions and only 1 on old ones.

It's important to support customers, which can't upgrade their Node.js version, but those customer won't likely upgrade their NPM packages like picocolors either. Upgrading anything in an environment using Node.js 1§6 would be risky and probably not worth it.

@prantlf
Copy link
Author

prantlf commented Jan 26, 2025

On the subject, I honestly can hardly understand the reason you'd want to import every single color separately. It just puzzles me how this can be convenient.

Other developers gave their opinions too, let me add one more. I work on both, browser and Node.js projects. I use the same coding standard. Browser projects enjoy tree-shaking. Node.js projects usually don't. (But it's possible to bundle Node.js projects too.) But the point is the code consistence and coding style, which mine prefers named exports. I'm not alone, you might see preferring named export as a kind of "industry standard" nowadays.

It doesn't mean that default exports are bad. It's just that named exports should work too, like they do with other NPM packages.

With default export I don't need to go back and forth updating import (even if auto import enabled in an editor of choice). This feels like an unnecessary mental overhead.

As you said, autoimporting solves it with no mental overhead. And autoimporting is enabled by default in modern IDEs. It's the way how to code more efficiently.

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.

Build impossible with rollup and typescript Avoid forcing allowSyntheticDefaultImports flag
4 participants