React-typescript compiler should not add dependencies on react

KutnerUri commented 4 years ago

In my project, I have many components. Some of them are react, and some are plain logic.

I set react-typescript compiler for the whole project. (after ejecting, I can micro manage the compiler, but in the author environment, it's a lot of overhead)

I expect the compiler to only transpile the source code and generate d.ts. Instead, the component is added a new dependencies: 'react' and 'react-dom'.

consider this code:

import react from 'react';

export default () => { return <div>hello</div>; }

In this case, I expect only React to be a dependency. (it's ok if it is down-graded to 'peer-dependency')

also consider this code:

export default function add(a: number, b: number) { return a + b; }

Even when using react-typescript compiler, there should be no dependency on 'react'. I know it counter-intuitive, but I think react-typescript has the potential to cover my whole project, instead of having to specify typescript and react-typescript for each component.

reference

compiler: bit.envs/compilers/react-typescript@3.1.7

bit version : 14.5.0 node version : v12.12.0 npm version : 6.11.3 yarn version : 1.19.1 platform : darwin

GiladShoham commented 4 years ago

I think it's better to namespace your components and change to the regular ts compiler for those who don't need react.

qballer commented 4 years ago

In terms of approach we need to look at this differently. Were writing an environment. A tool for making the UI component reusable. It's a build process I agree, but by default environment !== compiler. It's more then that.

I think I don't have access to the dependencies identified by bit in getDynamicDependencies. If/when I do, I should adjust dependencies only when identified by bit.

qballer commented 4 years ago

@KutnerUri if I have dependencies that if exists I forceToPeer. Is that ok? for instance I might need @types/node as dependency for the consumer environment. even though it's a devDependency in the author environment.

thoughts?

KutnerUri commented 4 years ago

As a rule, I think Bit should be handling the dependency detection.

I guess the 'react' compiler is specific enough to bump react to a peer dependency, but it should respect the author package.json. For example, if there is no dep on 'react', how is the compiler to know what version to install?

Maybe we can add a pre-build test checking that 'react' is a peer dependency? (only when imported) Then the user will get a warning and 'fix' his project, rather than having a decision forced on him by the compiler.

@types/node

something is weird about how we use this package. memfs has devDependency on "@types/node": "10.17.6", and I can see /// <reference types="node" /> at node_modules/memfs/lib/node.d.ts.

So why do we get errors in our components but not for memfs? maybe it is because devDeps are not installed in regular npm install?

qballer commented 4 years ago

As a rule, I think Bit should be handling the dependency detection.

Bit can't handle dependencies which the compiler needs like babel plugins. Forcing a dependency to a specific type (like toPeer, toDev, toDep - for regular) makes sense in some use cases. For example

toDep - @types/something should be a regular dependency so people consuming the component from the registry will be able to build. Needed when an API exposes types from @types/something.
toPeer- Bit will see react as a regular dep. We want to accommodate our users and set it to peer for them. it will help reusing.

So while bit does most of the work some of it should be in envs. Employing the strategy only if that dependency is present, does makes sense. I think I will adopt this.

@types/node

this isn't specificaly related to this issue. If you have a problem with types/node usage we should talk about it in a different issue.

KutnerUri commented 4 years ago

@types/something should be a regular dependency so people consuming the component from the registry will be able to build.

I think having @types/ as devDep is the common practice. Maybe @amitgilad3 knows about this?

Bit will see react as a regular dep. We want to accommodate our users and set it to peer for them.

Still, I think this should be a warning. It will encourage the user to fix the problem rather than go around it.

Employing the strategy only if that dependency is present, does makes sense

excellent!

qballer commented 4 years ago

@types/something should be a regular dependency so people consuming the component from the registry will be able to build.

I think having @types/ as devDep is the common practice. Maybe @amitgilad3 knows about this?

I have checked this. It makes sense to place most @types files in devDependencies unless your API exposes a type from that dependency. In that case you want the type to be a regular dependency so typescript project consuming the package will be able to build on top of it - and find that internal type.

teambit / envs