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

Fix: add universal export to root #57

Merged
merged 7 commits into from
Oct 14, 2021
Merged

Fix: add universal export to root #57

merged 7 commits into from
Oct 14, 2021

Conversation

SimenB
Copy link
Contributor

@SimenB SimenB commented Oct 11, 2021

Jest doesn't support package exports (yet), so if you currently try to load eslint from within Jest (such as for running ruleTester) it fails to resolve unless the user adds some config like

{
  "jest": {
    "moduleNameMapper": {
      "@eslint/eslintrc/universal": "@eslint/eslintrc/dist/eslintrc-universal.cjs"
    }
  }
}

It would be nice if it just worked 🙂

I very much understand if you don't wanna add this hack for Jest's lack of support for package exports, but since it's not a big burden I have some hope 👍

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 11, 2021

CLA Signed

The committers are authorized under a signed CLA.

@eslint-github-bot
Copy link

Hi @SimenB!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag must be one of the following:

    The Tag is one of the following:

    • Fix - for a bug fix.
    • Update - either for a backwards-compatible enhancement or for a rule change that adds reported problems.
    • New - implements a new feature.
    • Breaking - for a backwards-incompatible enhancement or feature.
    • Docs - changes to documentation only.
    • Build - changes to build process only.
    • Upgrade - for a dependency upgrade.
    • Chore - for anything that isn't user-facing (for example, refactoring, adding tests, etc.).

    You can use the labels of the issue you are working on to determine the best tag.

  • There should be a space following the initial tag and colon, for example 'New: Message'.

  • The first letter of the tag should be in uppercase

Read more about contributing to ESLint here

@SimenB SimenB changed the title fix: add universal export to root Fix: add universal export to root Oct 11, 2021
@nzakas
Copy link
Member

nzakas commented Oct 12, 2021

This was brought up during the beta period so we assumed Jest would resolve the issue before the final release came out. What’s your timeline for the fix?

@ljharb
Copy link
Contributor

ljharb commented Oct 12, 2021

Unfortunately there’s no timeline yet for resolve, which is what jest uses currently - that doesn’t preclude jest fixing it faster.

I’ve been trying to find the time for it for awhile :-/ node also keeps altering the exports field semantics, which makes it harder.

@btmills
Copy link
Member

btmills commented Oct 12, 2021

Espree, the default parser, also uses package exports now. If we include this workaround, would we need to do the same thing in that project too?

@SimenB
Copy link
Contributor Author

SimenB commented Oct 12, 2021

This was brought up during the beta period so we assumed Jest would resolve the issue before the final release came out. What’s your timeline for the fix?

As Jordan mentions, Jest uses resolve which currently doesn't support it. I might be able to land a workaround for the default export (i.e. ., so not even applicable to this specific case) in jestjs/jest#11919, but that's a bit hacky and as I mention there I'm not a huge fan of the default semantics of resolve.exports so even that is sorta in in limbo atm. So I don't really have a timeline here, unfortunately. 🙁

Espree, the default parser, also uses package exports now. If we include this workaround, would we need to do the same thing in that project too?

No, since espree only uses exports to do conditionals on the entry point plus package.json, and it still specifies a main entry which is CJS, that still works without an issue. If you were to add some other export (like /universal here) it would fail, but the way espree is now is not an issue due to the presence of the main field 🙂


If consumers of Jest wanna use native ESM this would fall apart of course, but due to the APIs jest use in Node still being flagged (and probably will be for some time) I assume most people (by far) still use CJS (or TS/Babel compiled ESM)

@SimenB
Copy link
Contributor Author

SimenB commented Oct 12, 2021

I added a comment explaining some of this in the file, which might include a non-starter - you use type: "module" but this is a .js file so technically cannot use require in it at all 😅 As mentioned there, Jest doesn't support type field in package.json outside of ESM mode (https://github.com/facebook/jest/blob/64de4d7361367fd711a231d25c37f3be89564264/packages/jest-resolve/src/shouldLoadAsEsm.ts#L31-L33) which is a bug I should probably fix, at which point even this workaround would stop working. But this would still work for other versions of Jest (if I can even fix it without proper exports support, at which point the workaround will be moot anyways)

@SimenB
Copy link
Contributor Author

SimenB commented Oct 12, 2021

FWIW I opened browserify/resolve#253 which might unblock Jest, but I'd still love to see this workaround land for all older versions of Jest anyways 🙂

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

I’m not thrilled about including a workaround like this because we won’t be able to remove it unless we include “drop support for Jest <x.y.z” as a breaking change. It’s unusual that Jest would be part of this package’s compatibility matrix. However, we’re trying to migrate to a new config system, so we’ll eventually stop working on this package, reducing the impact of this workaround being effectively permanent. Given this is the only way for all existing Jest versions to work with ESLint v8, and there’s no timeline for support without this workaround in future versions of Jest, I think it’s best for the community that we do this.

@nzakas
Copy link
Member

nzakas commented Oct 14, 2021

Agreed

@btmills btmills merged commit 6b5fc8b into eslint:main Oct 14, 2021
@SimenB SimenB deleted the patch-1 branch October 14, 2021 06:34
@SimenB
Copy link
Contributor Author

SimenB commented Oct 14, 2021

Thank you! ❤️

sweepline pushed a commit to sweepline/eslint-plugin-unused-imports that referenced this pull request Nov 10, 2021
* removes typescript devDependency

not used at all

Signed-off-by: Benjamin Kroeger <[email protected]>

* updates [email protected]

Signed-off-by: Benjamin Kroeger <[email protected]>

* chore(updates devDepencencies): eslint@^8.0.0

- drops support for node 10
- adds module name mapper for jest (see eslint/eslintrc#57)

BREAKING CHANGE

Signed-off-by: Benjamin Kroeger <[email protected]>

* upgrades eslint@^8.0.1

eslint/eslint@f9217e5
Signed-off-by: Benjamin Kroeger <[email protected]>

* removes trailing slash on bugs link

Signed-off-by: Benjamin Kroeger <[email protected]>
@SimenB
Copy link
Contributor Author

SimenB commented Apr 22, 2022

Circling back to this, Jest 28 has full support (afaik) of exports. Currently in alpha releases, but I just need to write the blog post and we're ready for a stable release. Hopefully done this weekend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants