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

ci: fix for windows #4229

Merged
merged 3 commits into from Feb 1, 2022
Merged

ci: fix for windows #4229

merged 3 commits into from Feb 1, 2022

Conversation

snitin315
Copy link
Member

@snitin315 snitin315 commented Jan 28, 2022

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Motivation / Use-Case

Breaking Changes

Additional Info

@snitin315 snitin315 closed this Jan 28, 2022
@snitin315 snitin315 reopened this Jan 28, 2022
@codecov
Copy link

codecov bot commented Jan 28, 2022

Codecov Report

Merging #4229 (bd2c165) into master (b9e11ba) will increase coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4229      +/-   ##
==========================================
+ Coverage   92.34%   92.47%   +0.12%     
==========================================
  Files          14       14              
  Lines        1541     1541              
  Branches      590      590              
==========================================
+ Hits         1423     1425       +2     
+ Misses        109      107       -2     
  Partials        9        9              
Impacted Files Coverage Δ
lib/servers/WebsocketServer.js 94.87% <0.00%> (+5.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9e11ba...bd2c165. Read the comment docs.

@alexander-akait
Copy link
Member

Weird 😕

@alexander-akait
Copy link
Member

We should build client using webpack v5 for webpack v4

@snitin315
Copy link
Member Author

How do we do that? It seems npm link webpack-dev-server will always run npm run build and we need to run npm link webpack-dev-server after installing webpack 4.

@alexander-akait
Copy link
Member

@snitin315 Let's install webpack v5 then build client, then install webpack v4 and skip client generation at all

package.json Outdated
@@ -25,7 +25,8 @@
"commitlint": "commitlint --from=master",
"build:client": "rimraf ./client/* && babel client-src/ --out-dir client/ --ignore \"client-src/webpack.config.js\" --ignore \"client-src/modules\" && webpack --config client-src/webpack.config.js",
"build:types": "rimraf ./types/* && tsc --declaration --emitDeclarationOnly --outDir types && node ./scripts/extend-webpack-types.js && prettier \"types/**/*.ts\" --write && prettier \"types/**/*.ts\" --write",
"build": "npm-run-all -p \"build:**\"",
"build": "npm run build:client",
"postbuild": "npm run-all -p \"build:**\"",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to keep this as it, there are bugs in yarn with this, just improve CI file

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm link webpack-dev-server is always running npm run build, and npm run build:types is failing with webpack-4

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this, we should search workarounds, we can try --ignore-scripts

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am searching for workarounds, We already have --ignore-scripts it only ignores pre and post scripts so it will ignore prebuild and postbuild scripts only.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems issue with the latest npm version, using node v14 with webpack 4 works fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory we create scripts which remove command from package.json and run it on CI

@alexander-akait
Copy link
Member

@snitin315 let's uncomment, we will fix flacky tests in future

test/e2e/server.test.js Outdated Show resolved Hide resolved
@snitin315 snitin315 marked this pull request as ready for review February 1, 2022 12:43
@snitin315 snitin315 changed the title ci: debug for windows ci: fix for windows Feb 1, 2022
@alexander-akait alexander-akait merged commit ad1d14c into master Feb 1, 2022
@alexander-akait alexander-akait deleted the ci-fix-windows branch February 1, 2022 13:51
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

Successfully merging this pull request may close these issues.

None yet

2 participants