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

Typescript classes not working in components-typescript #29

Closed
jmyrland opened this issue Jan 15, 2020 · 15 comments
Closed

Typescript classes not working in components-typescript #29

jmyrland opened this issue Jan 15, 2020 · 15 comments

Comments

@jmyrland
Copy link
Contributor

jmyrland commented Jan 15, 2020

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
Copy link
Contributor Author

jmyrland commented Jan 15, 2020

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
Copy link
Collaborator

F1LT3R commented Jan 15, 2020

Perhaps we can convince @pmoleri to look into this?

@pmoleri had PR'ed TypeScript into the codebase originally. See: react-workspaces/create-react-app#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
Copy link
Contributor Author

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
Copy link
Collaborator

F1LT3R commented Jan 15, 2020

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
Copy link

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
Copy link
Collaborator

F1LT3R commented Jan 16, 2020

Ah! That sounds right.

Interested in throwing a PR together?

jmyrland added a commit to jmyrland/react-workspaces-playground that referenced this issue Jan 16, 2020
@jmyrland
Copy link
Contributor Author

@F1LT3R : I've made a reproducable example here: jmyrland#1

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

@jmyrland
Copy link
Contributor Author

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

@abrkn
Copy link

abrkn commented Jan 16, 2020

Where are you placing this .env file?

@jmyrland
Copy link
Contributor Author

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

jmyrland added a commit to jmyrland/react-workspaces-playground that referenced this issue Jan 16, 2020
jmyrland added a commit to jmyrland/react-workspaces-playground that referenced this issue Jan 16, 2020
@jmyrland
Copy link
Contributor Author

@abrkn check out this commit: jmyrland@97e47f2

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 :)

jmyrland added a commit to jmyrland/react-workspaces-playground that referenced this issue Jan 16, 2020
+ Adds .env file to extend eslint
+ Adjusted eslintrc.js to be based on CRA by default
@F1LT3R
Copy link
Collaborator

F1LT3R commented Jan 16, 2020

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
Copy link
Contributor Author

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
Copy link
Collaborator

F1LT3R commented Jan 18, 2020

☝️ 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
Copy link

pmoleri commented Jan 20, 2020

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?

F1LT3R added a commit that referenced this issue Jan 29, 2020
@F1LT3R F1LT3R closed this as completed Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants