-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix: #1719 #2017
fix: #1719 #2017
Conversation
|
Codecov Report
@@ Coverage Diff @@
## master #2017 +/- ##
=========================================
- Coverage 92.77% 92.08% -0.7%
=========================================
Files 29 29
Lines 1149 1175 +26
Branches 327 335 +8
=========================================
+ Hits 1066 1082 +16
- Misses 79 88 +9
- Partials 4 5 +1
Continue to review full report at Codecov.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2017 +/- ##
==========================================
+ Coverage 92.77% 92.97% +0.20%
==========================================
Files 29 30 +1
Lines 1149 1182 +33
Branches 327 332 +5
==========================================
+ Hits 1066 1099 +33
Misses 79 79
Partials 4 4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idea is good, but implementation is not good
Also we need tests
client-src/default/index.js
Outdated
const parsedQuery = querystring.parse(path); | ||
compilerName = parsedQuery.compilerName; | ||
} | ||
})(__resourceQuery) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to util
client-src/live/index.js
Outdated
@@ -60,6 +60,7 @@ $(() => { | |||
} | |||
}, | |||
ok() { | |||
console.log('OK!!'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you need What is OK!!
? It is misleading need better text
lib/Server.js
Outdated
@@ -719,7 +719,7 @@ class Server { | |||
return; | |||
} | |||
|
|||
this._sendStats([connection], this.getStats(this._stats), true); | |||
this._sendStats(null, [connection], this.getStats(this._stats), true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null?
lib/utils/updateCompiler.js
Outdated
return ((1 + Math.random()) * 0x10000 | 0).toString(16).substring(1); | ||
} | ||
return s4() + s4() + '-' + s4() + '-' + s4() + '-' + s4() + '-' + s4() + s4() + s4(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better use package for this
Agreed. Rather than modifying both server and client code, distinguishing websocket messaging like using channel by modifying only server code (and minimum client code modification as possible) seems to be a better implementation. I was kind of in a hurry so the implementation is actually dirty. I'll improve it with further commits :) |
Only server code is modified rather than modifying both (client and server) code. Each compiler creates its own socket server With SockJS implementation, Servers are distinguished by prefix which is the name of the compiler. If compiler name is not provided in webpack option, Any random GUID will be generated and named.
The new commit remains client code unchanged (so the code is completely same with the upstream master code) and only server code is changed. Please see the commit (even it contains tiny errors and corrected by the next commits) I am trying to write and run tests, but running test keeps failing. It seems like Windows related error. I'm using VSCode in Windows 10 and if any code change happens with Windows environment editor, ESLint test goes fail, with unknown error. Could you check if error happens on test with my repository source? |
What is error(s) you got? |
Actually the error is much same with what Azure pipeline did. It seems error happens on Lint:prettier |
You can run tests using |
fix for #1719
When sending a 'ok' and 'invalid' message, compiler name is passed as a data.
compiler name can be defined in config options. If user does not provided it, the script generates a random guid as a compiler name.