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

@lingui/macro has undeclared dependency on @lingui/react #936

Closed
mjpieters opened this issue Jan 9, 2021 · 14 comments · Fixed by #973
Closed

@lingui/macro has undeclared dependency on @lingui/react #936

mjpieters opened this issue Jan 9, 2021 · 14 comments · Fixed by #973

Comments

@mjpieters
Copy link

Describe the bug

You can't use @lingui/macro without installing react, because packages/macro/index.d.ts pulls in react and @lingua/react:

import type { ReactElement, ComponentType, ReactNode } from "react"
import type { MessageDescriptor } from "@lingui/core"
import type { TransRenderProps } from "@lingui/react"

However, the dependencies are not listed in package.json.

To Reproduce

Install @lingui/core and @lingui/macro, but no other packages into a (pure) TypeScript project and try to use:

import { t } from "@lingui/macro"

Expected behavior

Either the dependency should be made clear or the package should be usable without react.

Additional context
Add any other context about the problem here.

  • jsLingui version lingui --version: 3.3.0
@semoal
Copy link
Contributor

semoal commented Jan 10, 2021

Lingui macro could be used on node environments where react is not required. I'm not sure how to fix this. Any recommendation?

@semoal
Copy link
Contributor

semoal commented Jan 10, 2021

We could add as peerDependency react but i'm worried about because on newer npm peerDependencies will be auto-downloaded so a lot of node projects will install react without the need

@amirqasemi74
Copy link

Yes I have this problem two... react usage must be removed from macro codes

@mjpieters
Copy link
Author

mjpieters commented Jan 11, 2021

The project currently can't be used at all without @lingui-react and react, so any project that actually uses it must be using react too.

Personally, I'd love to use macros, but my project doesn't use react. So if possible, I'd prefer it if the react portion was optional or moved to a dedicated, separate package.

@semoal
Copy link
Contributor

semoal commented Jan 11, 2021

After some investigation I found that theimport type statement says that a import type without that dependency will be converted to any and won't crash.
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-8.html#type-only-imports-and-export

I created this commands:

mkdir tsc-test
cd tsc-test/
touch index.ts
npm init --yes
npm install --save-dev typescript
npm install --save @lingui/macro
tsc --init
import { t } from "@lingui/macro"

const SOME_CODE = {
   hi: t`example`
}

And after running tsc it compiled properly. What typescript version are you using? This feature got introduced on 3.8, so probably that's the problem

@mjpieters
Copy link
Author

I use:

  • typescript 4.1.3
  • browserify 17.0.0
  • tsify 5.0.2
  • babelify 10.0.0
  • babel 7.12.10

with the browserify / tsify / babelify pipeline converting from typescript to browser-ready code. tsconfig.js contains:

{
  "compilerOptions": {
    "target": "esnext",
    "moduleResolution": "node",
    "module": "es6",
    "allowJs": true,
    "strict": true,
    "isolatedModules": true,
    "esModuleInterop": true
  }
}

(plus project-specific paths for types, rootDir and an exclude list next to the compilerOptions), and babel.config.json contains:

{
  "presets": ["@babel/env", "@babel/typescript"],
  "plugins": [
    "@babel/proposal-class-properties",
    "@babel/proposal-object-rest-spread",
    ["polyfill-regenerator", { "method": "usage-pure" }]
  ]
}

If I use:

import { t } from '@lingui/macro'

const SOME_CODE = {
  hi: t`example`
}

and add macros to my babel.conf.json config plugins list, I can't compile anymore, I get an error:

TypeScript error: /.../node_modules/@lingui/macro/index.d.ts(1,61): Error TS2307: Cannot find module 'react' or its corresponding type declarations.

So I do have to install react, I can't compile without it being installed.

I do understand that using import type ... only depends on the type information at compile time, no runtime dependency is pulled in. I can install react and @lingui/react and not have it end up in my output, but then you do need to list react as a dependency! I don't want to have to find out that I have to install react manually here.

@mjpieters
Copy link
Author

Correction: the dependency is on @types/react, not react.

@semoal
Copy link
Contributor

semoal commented Jan 13, 2021

Couldn't be the problem browserify or tsfiy doens't handle correctly yet import type statement and they are thinking that import is a required dependency?

I can compile perfectly with tsc your code, so..

@mjpieters
Copy link
Author

mjpieters commented Feb 1, 2021

I didn't have time until now to investigate further. The difference between your test and my setup is a specific flag that tsc --init has set in tsconfig.json but isn't present in my project:

{
  "compilerOptions": {
    // ...
    /* Advanced Options */
    "skipLibCheck": true,                           /* Skip type checking of declaration files. */
    // ...
}

See the skipLibCheck docs:

Skip type checking of declaration files.

This can save time during compilation at the expense of type-system accuracy. [...] Rather than doing a full check of all d.ts files, TypeScript will type check the code you specifically refer to in your app’s source code.

If you set that to the default false, you'll see the same errors.

Starting from your example:

$ tsc  # no errors
$ sed -i '' -e 's/"skipLibCheck": true, /"skipLibCheck": false,/' tsconfig.json  # disable skipLibCheck
$ tsc
node_modules/@lingui/macro/index.d.ts:1:61 - error TS2307: Cannot find module 'react' or its corresponding type declarations.

1 import type { ReactElement, ComponentType, ReactNode } from "react"
                                                              ~~~~~~~

node_modules/@lingui/macro/index.d.ts:2:40 - error TS2307: Cannot find module '@lingui/core' or its corresponding type declarations.

2 import type { MessageDescriptor } from "@lingui/core"
                                         ~~~~~~~~~~~~~~

node_modules/@lingui/macro/index.d.ts:3:39 - error TS2307: Cannot find module '@lingui/react' or its corresponding type declarations.

3 import type { TransRenderProps } from "@lingui/react"
                                        ~~~~~~~~~~~~~~~


Found 3 errors.

@semoal
Copy link
Contributor

semoal commented Feb 4, 2021

I've doing some investigation and you're right that with skipLibCheck false, it crashes. (Typescript default settings and the recommended one is to keep skipLibCheck to true. Anyways we could fix this...

1º From our side we could publish inside macro package a global.d.ts file without dependencies of react, or @lingui/react just @lingui/core.
2º Inside node environments you'll need just:
- lingui-env.d.ts file:
///<reference path="node_modules/@lingui/macro/node.d.ts" />
- Modifiy tsconfig.json: to include that file:
"include": [ "lingui-env.d.ts" ],

Next.js does something similar, so probably this could be a good option. What do you think @mjpieters

@mjpieters
Copy link
Author

Next.js does something similar, so probably this could be a good option. What do you think @mjpieters

I don't really have much of an opinion on how this is solved, sorry. :-) I'm still green around the ears when it comes to NPM / typescript packaging options.

@semoal
Copy link
Contributor

semoal commented Feb 7, 2021

I'll fix this in the next tuesday release, we'll going to do that create a node.d.ts types for node environments where react is not included, and if someone is using node environments only has to create that lingui-env.d.ts file or modifiying types: [] on tsconfig pointing that node.d.ts file

@semoal semoal self-assigned this Feb 7, 2021
@mjpieters
Copy link
Author

Note that I'm targeting the browser, but am just not using react. Targeting the "no react" version to be the same as "node only" may be confusing for others in the same situation.

@semoal
Copy link
Contributor

semoal commented Feb 8, 2021

Note that I'm targeting the browser, but am just not using react. Targeting the "no react" version to be the same as "node only" may be confusing for others in the same situation.

Yes probably I'll call it global.d.ts, because should work in any environment

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

Successfully merging a pull request may close this issue.

3 participants