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

Enhancement: [no-require-imports] Add option allowJson #5314

Closed
4 tasks done
remcohaszing opened this issue Jul 6, 2022 · 6 comments · Fixed by #7710
Closed
4 tasks done

Enhancement: [no-require-imports] Add option allowJson #5314

remcohaszing opened this issue Jul 6, 2022 · 6 comments · Fixed by #7710
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@remcohaszing
Copy link
Contributor

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the rule's documentation

https://typescript-eslint.io/rules/no-require-imports/

Description

If module in tsconfig.json is set to Node16, TypeScript 4.7 will convert import … = require('…') statements to require calls created using _createRequire.

As long as import assertions are not allowed in Node.js, I believe this is a nice syntax to read package.json files.

I propose to add an option named allowJson to no-require-imports that allows require imports of JSON files.

Fail

import lodash = require('lodash')
import lib = require('./lib')

Pass

These are currently also errored on


import lodashPkg = require('lodash/package.json')
import pkg = require('./package.json')

Additional Info

In:

import pkg = require('./package.json')

console.log(pkg.version)

Out:

import { createRequire as _createRequire } from "module";
const __require = _createRequire(import.meta.url);
const pkg = __require("./package.json");
console.log(pkg.version);

TypeScript playground

@remcohaszing remcohaszing added enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Jul 6, 2022
@Josh-Cena
Copy link
Member

As long as import assertions are not allowed in Node.js

Import assertions are stable and unflagged in Node. JSON modules are experimental but IMO there's little reason to not use it for non-library code.

@remcohaszing
Copy link
Contributor Author

As long as import assertions are not allowed in Node.js

Import assertions are stable and unflagged in Node. JSON modules are experimental but IMO there's little reason to not use it for non-library code.

I gave it a try and it’s not supported in Node.js 14.

It is supported in Node.js 18, but it prints a warning stating it’s experimental. This doesn’t look great for CLIs.

@Josh-Cena
Copy link
Member

Indeed, many reasons to stop using Node 14 😄 I do use it personally and I find the fact that the code is fully ESM more pleasing than that I get a warning.

@remcohaszing
Copy link
Contributor Author

I can’t argue with that!

However, Node.js 14 is still a supported LTS, and I believe this TypeScript syntax is a nice way to deal with JSON files for now.

@bradzacher
Copy link
Member

Given that node14 will EOL in april next year - is it worth adding an option to support the old way of doing things when people will be moving to the new way relatively soon?

Looking into it - it's supported in node 16 behind a flag, but node16 will EOL a few months after node 14. It's fully supported in node18, which will EOL in 3 years.

I'm not entirely convinced that an option is the best of ideas given the above and given how infrequent a usecase it is to directly import JSON.

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for maintainers to take a look labels Jul 9, 2022
@remcohaszing
Copy link
Contributor Author

I can think of one legitimate reason to import package.json, which is to use certain metadata inside a GUI About screen, CLI --version flag, collect metadata for issue reports or analytics. I think typically libraries should avoid importing JSON.

@JoshuaKGoldberg JoshuaKGoldberg added accepting prs Go ahead, send a pull request that resolves this issue and removed awaiting response Issues waiting for a reply from the OP or another party labels Nov 16, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants