react-workspaces / react-workspaces-playground

⚛️ 🐈 Zero Config Create-React-App Monorepos with Yarn Workspaces, Lerna and React Storybook.
https://react-workspaces.github.io/react-workspaces-playground/
764 stars 128 forks source link

Typescript classes not working in components-typescript #29

Closed jmyrland closed 4 years ago

jmyrland commented 4 years ago

First off, great job on this repository @F1LT3R ! 👏

I'm having some issues in regards to using classes in the shared components-typescript library.

If I add a class to the CompTwo.tsx component:

class Test {
  public prop: string;
  constructor(v: string) {
    this.prop = v;
  }
}

I get the following error when starting app-typescript:

Failed to compile.

./react-workspaces-playground/packages/components-typescript/src/CompTwo/CompTwo.tsx
  Line 5:10:  Parsing error: Unexpected token

  3 |
  4 | class Test {
> 5 |   public prop: string;
    |          ^
  6 |   constructor(v: string) {
  7 |     this.prop = v;
  8 |   }

Is there a quick fix to this issue, or some configurations that I am not aware of?

Related issues: #18, #28

jmyrland commented 4 years ago

By investigating further, I've found that this might be related to the lint module in webpack.config.js.

If I exclude typescript files from this step, the app is able to build (line 369):

        // First, run the linter.
        // It's important to do this before Babel processes the JS.
        {
-          test: /\.(js|mjs|jsx|ts|tsx)$/,
+          test: /\.(js|mjs|jsx)$/,

But it's not a viable option, as it disables all linting.

Another "fix", is to only include the source of the app in the lint step:

              loader: require.resolve('eslint-loader'),
            },
          ],
-          include: includePaths,
+          include: [includePaths[0]],

This is not ideal, but it keeps the lint rules for the application and allows typescript features (classes, casting, etc) in external components.

I have not found a solution that lints both the app and external components, while maintaining support for the above mentioned typescript features.

F1LT3R commented 4 years ago

Perhaps we can convince @pmoleri to look into this?

@pmoleri had PR'ed TypeScript into the codebase originally. See: https://github.com/react-workspaces/create-react-app/pull/3

I'm not a TypeScript developer, so perhaps I'm going to be slow debugging this one. :(

@jmyrland - Do you think the errors are actually being thrown at the linting stage or the transpile stage, or both?

Also...

Is it possible this is an issue with your main:src key in some of the package.json files, or your "workspaces": {"package-entry: "..."} key in the root package.json?

Ie: is it possible you are using "main:src": "src/index.js" where you should be using "main:src": "src/index.tsx"?

See:

It is also possible that this is a linting configuration. We have linting installed in root, but I had set that up before TypeScript, so perhaps something is off.

If you have a repo somewhere I can pull your setup, I will see if I can get it running.

jmyrland commented 4 years ago

Thanks for your reply! I'm writing this "on the go" so I do not have all the details regarding your questions - but I will provide this as soon as I'm in front of my computer 🙂

I believe it's only affecting the lint stage, as the code executes correctly (but I'm not 100% certain)

If you drop in that class in one of the components (in the components-typescript package), an error will be displayed when running app-typescript so it is easily reproducible. I believe it is the same case regarding #28. I'll fork the repo and give you a link for a reproducible case 👍

I am not that familiar with the CRA webpack config, so I am not exactly sure how to fix this 100% but the last adjustment to the config works in my case (it runs the app correctly) - but I do not know what side effects this change has (besides not linting external components).

I'm certain there is a better solution to this problem. I'll be happy to help however I can 🙂

F1LT3R commented 4 years ago

Thanks for your help @jmyrland! (and the ref to #28)

If it works in Vanilla CRA, I'm convinced it should work in this playground repo too, with some work on the configuration.

SyedRFarhan commented 4 years ago

I didn't understand the problem in my previous comment so I deleted it. But I think I've found a solution.

By can setting the environment variable EXTEND_ESLINT to true, CRA will pick up the eslintrc.js config file at the root of app-typescript. The app will compile now. To get all the linting rules that CRA has we can add react-app to extends. After this the app will compile + show the linting warnings in the console.

The react-app configuration sets an override for .ts/tsx files using the glob: files: ['**/*.ts?(x)']. I think it may be resolving from src, so it misses files outside of this. The eslintrc.js configured at the root of app-typescript always uses @typescript-eslint/parser so it catches tsx/ts files outside of src. This is my rationale at least.

F1LT3R commented 4 years ago

Ah! That sounds right.

Interested in throwing a PR together?

jmyrland commented 4 years ago

@F1LT3R : I've made a reproducable example here: https://github.com/jmyrland/react-workspaces-playground/pull/1

I'll try the changes suggested by @SyedRFarhan to see if this circumvents the errors 👍

jmyrland commented 4 years ago

After adding an .env file with EXTEND_ESLINT=true - it works without adjusting react-scripts 👍

abrkn commented 4 years ago

Where are you placing this .env file?

jmyrland commented 4 years ago

@abrkn I placed it inside the app-typescript package, alongside the package.json

jmyrland commented 4 years ago

@abrkn check out this commit: https://github.com/jmyrland/react-workspaces-playground/pull/1/commits/97e47f2243c01722a1eea8e245e609e59bc548e5

I've removed the "custom" eslint rules by default, and only extended the "standard" rules from CRA - as it might make it easier to migrate projects :)

F1LT3R commented 4 years ago

Is there any way to get the EXTEND_ESLINT=true or the same effect into a configuration file other than .env?

I suppose it doesn't matter for now, but I had previously thought the practice of relying on env files for your react app was discouraged?

Perhaps this was only discouraged for certain kinds of settings.

jmyrland commented 4 years ago

It is an opiniated topic 😁 This is what the official CRA docsstates regarding .env-files :

.env files should be checked into source control (with the exclusion of .env*.local).

F1LT3R commented 4 years ago

:point_up: makes sense

I had encountered CRA Monerepo configuration issues when previously using incompatible versions of things like eslint and WebPack that were clobbering each other when not being hoisted, or the setup had apps using multiple versions of CRA.

See: https://f1lt3r.io/create-react-app-monorepo-with-lerna-storybook-jest#create-your-react-app

Package versions are a much more dangerous class of problem. But I can't think of a good reason why your use of the .env file for the listing extension would be a problem.

pmoleri commented 4 years ago

Hi. When I first implemented the changes for TypeScript I found this same issue. At that time I decided to disable linting outside the main project. I guess that this change was overwritten when updating to CRA 3.0.

It seems that your solution is even better because it actually lints the dependent code, right?