Convert core components to TypeScript (Round 2)

[trblackw]

@nickmcmillan @vaibhavhrt Okay, everything now should be able to be merged smoothly considering index.js (now index.tsx) is hook-based. I apologize for the merge conflict hiccup.

[nickmcmillan]

@trblackw thanks for your work on this (and no stress about the merge conflict)!

However I'm having a little trouble getting this branch to run. I'm seeing the message ../dist/index.es.js Module not found: Can't resolve './styles.css'.

I've run yarn to ensure all dependencies are updated. Is there something else I'm missing?

[trblackw]

Yeah I attempted to address that my adding the resolve plugin to the roll up config. TypeScript doesn’t like the import statement when importing files that aren’t .ts/.tsx/.js. The require syntax that I used at the top of the refactored index.tsx combined with the plugin added to the roll up config should have fixed this 🧐 I can look into it

On Mon, Jun 3, 2019 at 1:02 PM Nick McMillan notifications@github.com wrote:

@trblackw https://github.com/trblackw thanks for your work on this (and no stress about the merge conflict)!

However I'm having a little trouble getting this branch to run. I'm seeing the message ../dist/index.es.js Module not found: Can't resolve './styles.css'.

I've run yarn to ensure all dependencies are updated. Is there something else I'm missing?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nickmcmillan/react-physics-dragger/pull/9?email_source=notifications&email_token=AIVCT26UVGMPBOB3K42Q3KDPYVFB7A5CNFSM4HSBJV22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW2BNYY#issuecomment-498341603, or mute the thread https://github.com/notifications/unsubscribe-auth/AIVCT22IMZNAZM3EG2ZLOWTPYVFB7ANCNFSM4HSBJV2Q .

[trblackw]

This might help https://link.medium.com/wE7MP8SVdX

I’m away from my computer for the next day so (vacation from work) if you could maybe find a solution before then maybe that would be easier? Otherwise I can address it in the next couple days

On Mon, Jun 3, 2019 at 1:06 PM Tucker Blackwell trblackw@ncsu.edu wrote:

Yeah I attempted to address that my adding the resolve plugin to the roll up config. TypeScript doesn’t like the import statement when importing files that aren’t .ts/.tsx/.js. The require syntax that I used at the top of the refactored index.tsx combined with the plugin added to the roll up config should have fixed this 🧐 I can look into it

On Mon, Jun 3, 2019 at 1:02 PM Nick McMillan notifications@github.com wrote:

@trblackw https://github.com/trblackw thanks for your work on this (and no stress about the merge conflict)!

However I'm having a little trouble getting this branch to run. I'm seeing the message ../dist/index.es.js Module not found: Can't resolve './styles.css'.

I've run yarn to ensure all dependencies are updated. Is there something else I'm missing?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nickmcmillan/react-physics-dragger/pull/9?email_source=notifications&email_token=AIVCT26UVGMPBOB3K42Q3KDPYVFB7A5CNFSM4HSBJV22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW2BNYY#issuecomment-498341603, or mute the thread https://github.com/notifications/unsubscribe-auth/AIVCT22IMZNAZM3EG2ZLOWTPYVFB7ANCNFSM4HSBJV2Q .

[vaibhavhrt]

@trblackw waiting for you to complete this before I can start working on #11

[trblackw]

I can’t seem to replicate the issue you were having on my end. When are you seeing the error message you listed above? When running yarn start? I just pushed a change to include .css extensions in the path options within the rollup config. Let me know if this resolves the issue on your end.

On Wed, Jun 12, 2019 at 12:55 AM Vaibhav Vishal notifications@github.com wrote:

@trblackw https://github.com/trblackw waiting for you to complete this before I can start working on #11 https://github.com/nickmcmillan/react-physics-dragger/issues/11

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nickmcmillan/react-physics-dragger/pull/9?email_source=notifications&email_token=AIVCT27LN7JLYR2QUSMR7U3P2B6VFA5CNFSM4HSBJV22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXPH2LQ#issuecomment-501120302, or mute the thread https://github.com/notifications/unsubscribe-auth/AIVCT25LE2TNARVLKUJM72LP2B6VFANCNFSM4HSBJV2Q .

[vaibhavhrt]

@trblackw you need to merge upstream/master

[trblackw]

@nickmcmillan Forgive me because I'm not very familiar with working with forked copies of repos. I'm currently on master of my fork and getting this error when attempting to merge:

I'm assuming there's some precursor steps I need to follow in order to be able to execute that command successfully?

edit: I did however just git pull git@github.com:nickmcmillan/react-physics-dragger.git and then resolve any merge conflicts and push.

[vaibhavhrt]

Make sure you are on branch master:

git checkout master

Fetch remote:

git fetch

Merge remote:

git merge upstream/master

[trblackw]

@vaibhavhrt What's weird is git fetch did not result in the fetching of the origin branch, which is why running git merge upstream/master was resulting in the screen shot I added above, I presume. I'm a bit of a git noob as well so I probably messed something up along the way. I see the conflicts have been resolved after what I just pushed, does that mean we're good to go?

I am noticing that Travis CI isn't very happy though

These environment variables weren't something I messed with on my end

[vaibhavhrt]

In conflict resolving you probably forced your changes instead of accepting incoming changes so eslint is giving errors. Go src/package.json, find the lintJs commands in scripts section and add fix option to it:

"lintJs": "eslint **/*.{js,jsx} --fix"

Run it: npm run lintJs, it will fix travis errors. undo changes in package.json, so it should be "lintJs": "eslint **/*.{js,jsx}" again. Also make sure you have deleted src/index.js

[trblackw]

Okay will have this done by 12:00 EST. Apologies for making this more difficult than it should be.

On Wed, Jun 12, 2019 at 7:23 AM Vaibhav Vishal notifications@github.com wrote:

In conflict resolving you probably forced your changes instead of accepting incoming changes so eslint is giving errors. Go package.json, find the lintJs commands in scripts section and add fix option to it:

"lintJs": "eslint */.{js,jsx} --fix"

Run it: npm run lint, it fix fix travis errors. undo changes in package.json, so it should be "lintJs": "eslint */.{js,jsx}" again. Also make sure you have deleted src/index.js

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nickmcmillan/react-physics-dragger/pull/9?email_source=notifications&email_token=AIVCT2Y2MBKKALA7DGCGWBLP2DMCFA5CNFSM4HSBJV22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXQC2WA#issuecomment-501230936, or mute the thread https://github.com/notifications/unsubscribe-auth/AIVCT2YAI4B4EW765KAYM23P2DMCFANCNFSM4HSBJV2Q .

[vaibhavhrt]

Actually forget my last comment, you actually messed up eslint settings, nothing wrong in resolving conflicts. We were using 2 spaces for tabs, you changed it 4 and trailing comma was allowed, you removed it too. See my review for fixing it.

[trblackw]

👍🏼

On Wed, Jun 12, 2019 at 7:39 AM Vaibhav Vishal notifications@github.com wrote:

Actually forget my last comment, you actually messed up eslint settings, nothing wrong in resolving conflicts. We were using 2 spaces for tabs, you changed it 4 and trailing comma was allowed, you removed it too. See my review for fixing it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nickmcmillan/react-physics-dragger/pull/9?email_source=notifications&email_token=AIVCT22PL5PXGCD36XXCQNTP2DN7LA5CNFSM4HSBJV22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXQD7PQ#issuecomment-501235646, or mute the thread https://github.com/notifications/unsubscribe-auth/AIVCT23TC5HHFXG7645MRVDP2DN7LANCNFSM4HSBJV2Q .

[trblackw]

Okay I won’t be able to push until 11:00 EST so I’ll let you know when I do

On Wed, Jun 12, 2019 at 7:43 AM Vaibhav Vishal notifications@github.com wrote:

@vaibhavhrt requested changes on this pull request.

Fix these 2, run npm run lintJs to make sure there are no more errors, if there is fix them.

In .eslintrc https://github.com/nickmcmillan/react-physics-dragger/pull/9#discussion_r292867934 :

• },
• "plugins": [
• "react"
• ],
• "parserOptions": {
• "sourceType": "module"
• },
• "rules": {
• // don't force es6 functions to include space before paren
• "space-before-function-paren": 0,
• // allow specifying true explicitly for boolean props
• "react/jsx-boolean-value": 0,
• // allow comma-dangle in multiline objects & arrays
• "comma-dangle": ["error", "only-multiline"]

This line needs to be there in add it back.

"comma-dangle": ["error", "only-multiline"]

---

In .eslintrc https://github.com/nickmcmillan/react-physics-dragger/pull/9#discussion_r292868512 :

• "rules": {
• // don't force es6 functions to include space before paren
• "space-before-function-paren": 0,
• // allow specifying true explicitly for boolean props
• "react/jsx-boolean-value": 0,
• // allow comma-dangle in multiline objects & arrays
• "comma-dangle": ["error", "only-multiline"]
• } -}
• "parser": "@typescript-eslint/parser",
• "extends": [
• "standard",
• "standard-react",
• "plugin:@typescript-eslint/recommended"

Extending this rule is probably forcing 4 spaces for indent. Add this line in rules section to make tab size 2 spaces:

"indent": ["error", 2],

See https://stackoverflow.com/a/48906878/9321755 for more details.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nickmcmillan/react-physics-dragger/pull/9?email_source=notifications&email_token=AIVCT25XSI6QCQ7FXU4RALLP2DOMLA5CNFSM4HSBJV22YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3JOJGQ#pullrequestreview-248702106, or mute the thread https://github.com/notifications/unsubscribe-auth/AIVCT2Y3FWYMMYF7YZJQQ4LP2DOMLANCNFSM4HSBJV2Q .

[vaibhavhrt]

K, let me know if there are still any errors

On Wed, 12 Jun 2019, 5:22 pm Tucker Blackwell, notifications@github.com wrote:

Okay I won’t be able to push until 11:00 EST so I’ll let you know when I do

On Wed, Jun 12, 2019 at 7:43 AM Vaibhav Vishal notifications@github.com wrote:

@vaibhavhrt requested changes on this pull request.

Fix these 2, run npm run lintJs to make sure there are no more errors, if there is fix them.

In .eslintrc < https://github.com/nickmcmillan/react-physics-dragger/pull/9#discussion_r292867934

:

• },
• "plugins": [
• "react"
• ],
• "parserOptions": {
• "sourceType": "module"
• },
• "rules": {
• // don't force es6 functions to include space before paren
• "space-before-function-paren": 0,
• // allow specifying true explicitly for boolean props
• "react/jsx-boolean-value": 0,
• // allow comma-dangle in multiline objects & arrays
• "comma-dangle": ["error", "only-multiline"]

This line needs to be there in add it back.

"comma-dangle": ["error", "only-multiline"]

---

In .eslintrc < https://github.com/nickmcmillan/react-physics-dragger/pull/9#discussion_r292868512

:

• "rules": {
• // don't force es6 functions to include space before paren
• "space-before-function-paren": 0,
• // allow specifying true explicitly for boolean props
• "react/jsx-boolean-value": 0,
• // allow comma-dangle in multiline objects & arrays
• "comma-dangle": ["error", "only-multiline"]
• } -}
• "parser": "@typescript-eslint/parser",
• "extends": [
• "standard",
• "standard-react",
• "plugin:@typescript-eslint/recommended"

Extending this rule is probably forcing 4 spaces for indent. Add this line in rules section to make tab size 2 spaces:

"indent": ["error", 2],

See https://stackoverflow.com/a/48906878/9321755 for more details.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub < https://github.com/nickmcmillan/react-physics-dragger/pull/9?email_source=notifications&email_token=AIVCT25XSI6QCQ7FXU4RALLP2DOMLA5CNFSM4HSBJV22YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3JOJGQ#pullrequestreview-248702106 , or mute the thread < https://github.com/notifications/unsubscribe-auth/AIVCT2Y3FWYMMYF7YZJQQ4LP2DOMLANCNFSM4HSBJV2Q

.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nickmcmillan/react-physics-dragger/pull/9?email_source=notifications&email_token=AFMJMLHPZNNI5I3TQKKF6WDP2DPP5A5CNFSM4HSBJV22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXQE7CY#issuecomment-501239691, or mute the thread https://github.com/notifications/unsubscribe-auth/AFMJMLAZ4Q6YGOIKUYLOLSTP2DPP5ANCNFSM4HSBJV2Q .

[trblackw]

@vaibhavhrt okay I updated the eslint settings and fixed the affected files. I also just removed the tslint plugin because it was directly conflicting with eslint.

[trblackw]

Okay. I’m offline again for a bit so I’ll address this this afternoon

On Wed, Jun 12, 2019 at 11:52 AM Vaibhav Vishal notifications@github.com wrote:

@vaibhavhrt requested changes on this pull request.

Also add react-scripts-ts as devDependency and then update the test scripts(in package.json) to run ts tests, right now it's not finding any tests as it's looking for tests with extension js.

"test": "cross-env CI=1 react-scripts-ts test --env=jsdom", "test:watch": "react-scripts-ts test --env=jsdom",

---

In package.json https://github.com/nickmcmillan/react-physics-dragger/pull/9#discussion_r292990023 :

\ No newline at end of file

• ],
• "dependencies": {
• "@types/jest": "^24.0.13",
• "@types/react": "^16.8.19",
• "@types/react-dom": "^16.8.4"

@types dependencies are devDependencies as well.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nickmcmillan/react-physics-dragger/pull/9?email_source=notifications&email_token=AIVCT27ZBTGWUXBK6Q32L2DP2ELU5A5CNFSM4HSBJV22YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3KUBPI#pullrequestreview-248856765, or mute the thread https://github.com/notifications/unsubscribe-auth/AIVCT247ANO5XK4BHHFI7T3P2ELU5ANCNFSM4HSBJV2Q .

[trblackw]

@vaibhavhrt let me know if there's anything else I need to do

[vaibhavhrt]

Also add react-scripts-ts as devDependency and then update the test scripts(in package.json) to run ts tests, right now it's not finding any tests as it's looking for tests with extension js.

"test": "cross-env CI=1 react-scripts-ts test --env=jsdom",
"test:watch": "react-scripts-ts test --env=jsdom",

[trblackw]

Oops, forgot that one. Should be good now @vaibhavhrt

[vaibhavhrt]

Ok looks like ts requires a tsconfig.test.json to be able to run test suite. Add a tsconfig.test.json in root directory with following contents.

{
  "extends": "./tsconfig.json",
  "compilerOptions": {
    "module": "commonjs"
  }
}

[trblackw]

@vaibhavhrt done

[vaibhavhrt]

@trblackw test is now running which is good, but it's failing. I am not sure what's wrong, might need some investigation. Can you try to figure out what's wrong. You can run tests locally by npm run test to make sure it passes before pushing commits. You can see error details by running tests locally or see the logs on tavis.

[trblackw]

I’ll take a look when I get a chance

On Thu, Jun 13, 2019 at 8:28 AM Vaibhav Vishal notifications@github.com wrote:

@trblackw https://github.com/trblackw test is now running which is good, but it's failing. I am not sure what's wrong, might need some investigation. Can you try to figure out what's wrong. You can run tests locally by npm run test to make sure it passes before pushing commits. You can see error details by running tests locally or see the logs on tavis.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nickmcmillan/react-physics-dragger/pull/9?email_source=notifications&email_token=AIVCT225ZBK43BSYVAM3MLDP2I4N3A5CNFSM4HSBJV22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXTQZXI#issuecomment-501681373, or mute the thread https://github.com/notifications/unsubscribe-auth/AIVCT262BIGV6W5CBQ3TKB3P2I4N3ANCNFSM4HSBJV2Q .

[trblackw]

@vaibhavhrt So I'm been trying to trouble shoot what's going on with the test that's failing in index.tsx, but I'm not super familiar with testing (sad face). Perhaps you might have a bit more insight into what's potentially going wrong here

What's confusing to me is that the error message: Unexpected token (221:12) is actually on line 248 in my editor (perhaps this is normal because extra spaces are removed upon compilation or something?). My editor shows no visible issues with the code. The result message of the test is Test suite failed to run, which seems to contradict the previous unexpected token issue?

What do you think?

[vaibhavhrt]

@trblackw I will clone your fork of this repo and take a look later. rn busy watching world cup

[trblackw]

@vaibhavhrt Okay after using the tsconfig.json you provided above the test is passing

[vaibhavhrt]

Looks good to me now. @nickmcmillan you can review and merge.

[nickmcmillan]

I'm still unable to get the example to run in this branch. Steps to reproduce are;

1. Checkout this branch, run yarn to ensure dependencies are updated
2. yarn start in the root folder
3. cd example and yarn start to launch the development example

At this point I see the error message I'd mentioned here; https://github.com/nickmcmillan/react-physics-dragger/pull/9#issuecomment-498341603 - which appears both in the terminal and in the browser.

4. I implemented the solution @vaibhavhrt suggested here; https://github.com/nickmcmillan/react-physics-dragger/pull/9#discussion_r294463338 which worked - I'm no longer seeing the error message.

In the browser, I'm seeing a new error message (screenshot attached) which suggests to me that Typescript is not being compiled, the browser is attempting to read TS directly.

Am I missing something? Have you both tried testing the project example as per step #3 above and if so did it work?

I've noticed that the tool used to bootstrap this project (https://github.com/transitive-bullshit/create-react-library) also has a Typescript scaffolding option. Is this something worth considering as a last resort? (ie re-scaffolding the project using create-react-library with TS enabled)

[vaibhavhrt]

@trblackw you can take a look at this repo, I am using ts there, it works (almost) perfectly, vaibhavhrt/react-github-buttons. My project was also bootstrapped using create-react-library but I used the typescript template, and manually updated all dev-dependencies.

[trblackw]

@vaibhavhrt Why not just use create-react-app with the --typescript flag? I've worked with TypeScript a lot, mostly with the foundation set up with CRA and have never had issues. @nickmcmillan I'm curious why you used create-react-library?

[vaibhavhrt]

@trblackw because it's a npm library not a react website/app. This repo is supposed to be pluggable into some other react app(may or may not be bootstrapped with CRA). When creating an app we have an index.html but not in a library like this, because it will be used in some app which will have it index.html. Web app (usually) have minified js, css etc, here in library we don't minify it. So that users can easily debug, and minify, gzip according to their choice. In library we publish (usually) index.js and index.es.js to enable tree shaking.

[trblackw]

Oh, well that makes sense then. Very cool. That’s a bit out of my scope, though, as far as how to go about resolving the current compile issue.

On Tue, Jun 18, 2019 at 1:39 PM Vaibhav Vishal notifications@github.com wrote:

@trblackw https://github.com/trblackw because it's a npm library not a react website/app. This repo is supposed to be pluggable into some other react app(may or may not be bootstrapped with CRA). When creating an app we have an index.html but not in a library like this, because it will be used in some app which will have it index.html. Web app (usually) have minified js, css etc, here in library we don't minify it. So that users can easily debug, and minify, gzip according to their choice. In library we publish (usually) index.js and index.es.js to enable tree shaking.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nickmcmillan/react-physics-dragger/pull/9?email_source=notifications&email_token=AIVCT25HOLBSNTY4HXDXR7LP3EMVTA5CNFSM4HSBJV22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX7M2ZA#issuecomment-503237988, or mute the thread https://github.com/notifications/unsubscribe-auth/AIVCT27GTU4VTXOAQMPB2I3P3EMVTANCNFSM4HSBJV2Q .

[vaibhavhrt]

I am guessing something is wrong in rollup.config Take a look at the rollup config of my repo and try to see what's different in them. I am not expert enough in ts to figure out the solution just by looking at error, so you try changing things in rollup.config If you can't figure it out I will clone your fork again and take a look.

[trblackw]

Okay I’ll take a look. Will get back to you some time tomorrow @vaibhavhrt

On Tue, Jun 18, 2019 at 1:52 PM Vaibhav Vishal notifications@github.com wrote:

I am guessing something is wrong in rollup.config Take a look at the rollup config of my repo and try to see what's different in them. I am not expert enough in ts to figure out the solution just by looking at error, so you try changing things in rollup.config If you can't figure it out I will clone your fork again and take a look.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nickmcmillan/react-physics-dragger/pull/9?email_source=notifications&email_token=AIVCT2YP35Q7ULK64LK5AHTP3EOEVA5CNFSM4HSBJV22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODX7N72A#issuecomment-503242728, or mute the thread https://github.com/notifications/unsubscribe-auth/AIVCT26EOJ4ZYEJEBJBZWILP3EOEVANCNFSM4HSBJV2Q .

[trblackw]

@vaibhavhrt Haven't had any luck. Maybe you could take a look

[vaibhavhrt]

ok, I will look when i get some time

[nickmcmillan]

I think I have a working solution. @vaibhavhrt and @trblackw would you mind taking a look at this PR; https://github.com/nickmcmillan/react-physics-dragger/pull/12 (which is branched of the work you've both done so far in this current PR)

Basically I re-initialised the create-react-library with TS enabled which provided a working foundation. I then copied the src files over from this PR, fixed a few errors, and it seems to be up and running ok (using the /example project).

@vaibhavhrt I don't think we need lintJs anymore and the eslint plugins that go with it. We can just use create-react-library's testing set up. So I've just made lintJS an alias for test so that Travis tests can pass.