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

Run lint on travis builds and use modern node versions #7490

Merged
merged 15 commits into from Nov 22, 2018

Conversation

@aaronraimist
Copy link
Contributor

aaronraimist commented Oct 13, 2018

Yes, this will create some noise but maybe that will actually cause someone to fix the lint problems.

@aaronraimist aaronraimist changed the title Run lintwithexclusions on travis builds, run travis on modern node versions Run lint on travis builds and use modern node versions Oct 13, 2018
@aaronraimist

This comment has been minimized.

Copy link
Contributor Author

aaronraimist commented Oct 13, 2018

If anyone knows why this won't build properly I'd love some help.

It seems to all start going wrong around https://travis-ci.org/vector-im/riot-web/jobs/440888490#L1226

ERROR in ./node_modules/matrix-js-sdk/browser-index.js
Module not found: Error: Can't resolve './lib/matrix' in '/home/travis/build/vector-im/riot-web/node_modules/matrix-js-sdk'
 @ ./node_modules/matrix-js-sdk/browser-index.js 1:15-38
 @ ./test/app-tests/joining.js
 @ ./test/app-tests sync \.jsx?$
 @ ./test/all-tests.js
ERROR in ./test/skin-sdk.js
Module not found: Error: Can't resolve 'matrix-react-sdk' in '/home/travis/build/vector-im/riot-web/test'
 @ ./test/skin-sdk.js 9:10-37
 @ ./test/app-tests/joining.js
 @ ./test/app-tests sync \.jsx?$
 @ ./test/all-tests.js

Seems like Setting up matrix-js-sdk and Setting up matrix-react-sdk aren't doing everything they are supposed to do?

EOF

./node_modules/.bin/eslint --no-ignore -f json src test |
jq -r '.[] | select((.errorCount) > 0) | .filePath' |

This comment has been minimized.

Copy link
@aaronraimist

aaronraimist Oct 13, 2018

Author Contributor

Even though I didn't use this here, I'd lobby for this change (only ignore files with errors, not errors and warnings) to be used here: https://github.com/matrix-org/matrix-react-sdk/blob/87bac56b9c3b0c8688835604a5e51a00220a4e37/scripts/generate-eslint-error-ignore-file#L19

Thoughts?

This comment has been minimized.

Copy link
@aaronraimist

aaronraimist Oct 13, 2018

Author Contributor

Right now lintwithexclusions in matrix-react-sdk makes it look like everything is ok when there are actually ✖ 765 problems (237 errors, 528 warnings). This change would at least make a bunch of those warnings more visible.

This comment has been minimized.

Copy link
@aaronraimist
@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Oct 18, 2018

@aaronraimist sorry for missing this. I suspect that fixing the merge conflict and merging in the latest develop would magically fix those errors when you get a chance.

@aaronraimist aaronraimist force-pushed the aaronraimist:lint branch from 20ffe20 to b388a11 Oct 19, 2018
@aaronraimist

This comment has been minimized.

Copy link
Contributor Author

aaronraimist commented Oct 19, 2018

@turt2live Still broken. Not sure what the problem is.

To fix the merge conflict, I updated package-lock.json by following this advice
npm/npm#18007 (comment) (running npm install --package-lock-only). Not totally sure why that added a bunch of new things.

Signed-off-by: Aaron Raimist <aaron@raim.ist>
Signed-off-by: Aaron Raimist <aaron@raim.ist>
…sions

I seemingly need babel-eslint version 8 for VectorHomePage.js but might as well just upgrade to version 10

Signed-off-by: Aaron Raimist <aaron@raim.ist>
@aaronraimist aaronraimist force-pushed the aaronraimist:lint branch from b388a11 to c470e2d Oct 20, 2018
Signed-off-by: Aaron Raimist <aaron@raim.ist>
@aaronraimist

This comment has been minimized.

Copy link
Contributor Author

aaronraimist commented Oct 21, 2018

@turt2live so it turns out I can reproduce this locally. It is some kind of issue with npm link, related to #7355?

I currently run (fish shell, ; and is like &&):

cd ../matrix-js-sdk/; and npm i; and cd ../matrix-react-sdk/; and npm i; and npm link ../matrix-js-sdk/; and cd ../riot-web/; and npm i; and npm link ../matrix-js-sdk/; and npm link ../matrix-react-sdk/

to install all the stuff based on the directions in the README.

After running that, the tests in matrix-react-sdk fail in the same way as they do in the travis build. If I run npm link ../matrix-js-sdk/in the matrix-react-sdk folder, the tests pass.

I'm hoping npm/rfcs#3 fixes this but is there something else I can do? I'm not sure why this is affecting only my PR.

@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Oct 21, 2018

I'm not too sure at a glance as to what could be going wrong there. I'll run the same tests on my end and see if I can reproduce it, and maybe find out why it is broken.

@turt2live turt2live self-assigned this Oct 23, 2018
@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Oct 25, 2018

Alright, so I fixed your original problem by introducing a completely different issue. When I saw this problem while writing matrix-org/matrix-js-sdk#764 an npm install fixed it. It's possible something in matrix-org/matrix-js-sdk#764 magically fixes the problem, but it is weird that the js-sdk (and therefore the react-sdk) failed to build at all.

@aaronraimist

This comment has been minimized.

Copy link
Contributor Author

aaronraimist commented Oct 27, 2018

:/

@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Oct 27, 2018

Great, so now it is on the original problem:

Error: Cannot find module 'escope'

bashes head against wall

@aaronraimist

This comment has been minimized.

Copy link
Contributor Author

aaronraimist commented Oct 27, 2018

Oh actually I think I can solve that, hang on

Signed-off-by: Aaron Raimist <aaron@raim.ist>
@aaronraimist

This comment has been minimized.

Copy link
Contributor Author

aaronraimist commented Oct 27, 2018

@aaronraimist

This comment has been minimized.

Copy link
Contributor Author

aaronraimist commented Oct 27, 2018

Travis is going to fail again until those are merged

fatal: Remote branch lint not found in upstream origin

@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Oct 27, 2018

Shouldn't a4ed1af require a change to the package lock too?

@aaronraimist

This comment has been minimized.

Copy link
Contributor Author

aaronraimist commented Oct 27, 2018

Yeah let me delete my package-lock.json and install again so I can commit that. It is currently all screwed up from trying to fix #6694.

Signed-off-by: Aaron Raimist <aaron@raim.ist>
@turt2live turt2live requested a review from vector-im/riot-web Oct 29, 2018
@turt2live turt2live removed their assignment Oct 29, 2018
@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Oct 29, 2018

Now that the build appears to be passing (at least on Node 8, as of writing this comment) I think it's safe to merge. Because it includes some of my commits though, it's best if someone else from the team reviews it.

@dbkr
dbkr approved these changes Nov 21, 2018
dbkr added 2 commits Nov 22, 2018
Remove node 6
@dbkr dbkr merged commit a7a5679 into vector-im:develop Nov 22, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.