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

[no-var-requires] add an option to ignore require('../package.json') #1902

Closed
bcoe opened this issue Apr 14, 2020 · 1 comment · Fixed by #7710
Closed

[no-var-requires] add an option to ignore require('../package.json') #1902

bcoe opened this issue Apr 14, 2020 · 1 comment · 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

@bcoe
Copy link

bcoe commented Apr 14, 2020

Repro

Create a typescript package that imports fields from package.json, e.g.,

import { engines as supportedEngines } from '../package.json';

Attempt to build and publish the module. You will get a build that looks something like this:

npm notice === Tarball Contents === 
npm notice 11.4kB LICENSE                                 
npm notice 1.9kB  build/package.json                      
npm notice 1.7kB  package.json                            
npm notice 1.2kB  README.md                               
npm notice 757B   build/src/backoff-timeout.d.ts          
npm notice 2.7kB  src/backoff-timeout.ts                  
npm notice 684B   build/src/call-credentials-filter.d.ts  
npm notice 2.5kB  src/call-credentials-filter.ts        

☝️ this bundle should include backoff-timeout.js, call-credentials-filter.js. There appears to be a bug (expected behavior?) with npm, such that if there's a package.json file in your build/ directory, no JavaScript is included in the publish.

Expected Result

@typescript-eslint/no-var-requires should not advise users to import from package.json, as it can lead to bad publications to npm.

Actual Result

@typescript-eslint/no-var-requires suggests that you switch any requires from package.json to imports.

Additional Info

here's where we ran into the issue: grpc/grpc-node#1357

Versions

N/A, this is related specifically to the @typescript-eslint/no-var-requires plugin, and is a suggestion for behavior.

Thoughts

To help protect folks from this hiccup, it might be nice if @typescript-eslint/no-var-requires was able to specifically suggest that folks use require on a package.json import, while suggesting import for other JSON files.

@bcoe bcoe added package: eslint-plugin-tslint Issues related to @typescript-eslint/eslint-plugin-tslint triage Waiting for maintainers to take a look labels Apr 14, 2020
@bradzacher
Copy link
Member

This is the same functionality that TS provides via its language server:
image

It's impossible for use (or TS) to know that:

  • your build will copy this file to your output folder
  • you intend to publish your package in a specific structure
  • based on the above, that npm will publish a bad package if the package.json is copied to your output folder.

Many of our users do not publish npm packages, or use webpack (or a similar build chain), or compile their files in place (etc - there are many permutations of things that will work fine) so this wouldn't necessarily cause problems for everyone.

happy to accept a PR to implement to not flag these, behind a default false option.

I would suggest that you file an issue with npm about this, and also potentially one with the typescript repo.

@bradzacher bradzacher added enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin and removed package: eslint-plugin-tslint Issues related to @typescript-eslint/eslint-plugin-tslint triage Waiting for maintainers to take a look labels Apr 14, 2020
@bradzacher bradzacher changed the title @typescript-eslint/no-var-requires leads to broken npm publications [no-var-requires] add an option to ignore require('../package.json') Apr 14, 2020
@JoshuaKGoldberg JoshuaKGoldberg added the accepting prs Go ahead, send a pull request that resolves this issue label Oct 25, 2021
@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
3 participants