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

Build script to setup ESM/CJS module exports #1185

Closed
wants to merge 1 commit into from

Conversation

gfortaine
Copy link

@gfortaine gfortaine commented Jan 15, 2023

fix #1159

@@ -0,0 +1,3 @@
const inquirer = require("./inquirer.js").default;
Copy link
Owner

Choose a reason for hiding this comment

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

This file sneaked in the diff (prob added/commited before the .gitignore change)

Copy link
Owner

Choose a reason for hiding this comment

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

Oh I see this is desired. Let's update the .gitignore so this file is included not ignored by Git.

import chalk from 'chalk';
import cliCursor from 'cli-cursor';
import figures from 'figures';
import chalk from '#chalk';
Copy link
Owner

Choose a reason for hiding this comment

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

Is there eslint rules to enforce to use the # imports if the dependency is found in the package.json import map. That would help enforce it for future collaborators.

Copy link
Owner

Choose a reason for hiding this comment

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

We'll also need to fix the node/no-missing-import eslint error before merging.

@@ -0,0 +1,3 @@
const inquirer = require("./inquirer.js").default;
Copy link
Owner

Choose a reason for hiding this comment

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

I'd recommend adding this file to the eslint ignore. It errors on CI now since it's using common.js syntax - and wouldn't compile as-is.

@SBoudrias
Copy link
Owner

Thanks a ton for that awesome PR! 🙏🏻 Let me know if you need help to resolve any issues

@SBoudrias
Copy link
Owner

Hey, so I tried to dig a bit that eslint issue. I've updated the master branch replacing eslint-plugin-node with eslint-plugin-n (fork that is maintained.)

This PR seems to imply ESM imports/exports map would now be taken into account. But I couldn't get them to pass on your branch - and I'm out of time to dig it deeper. Hopefully though this save you some time!

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.

Provide multi-module builds
2 participants