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

fix: #1719 #2017

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix: #1719 #2017

wants to merge 5 commits into from

Conversation

orange4glace
Copy link

  • 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

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.

@jsf-clabot
Copy link

jsf-clabot commented Jun 11, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@codecov
Copy link

codecov bot commented Jun 11, 2019

Codecov Report

Merging #2017 into master will decrease coverage by 0.69%.
The diff coverage is 92.68%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
lib/utils/updateCompiler.js 100% <100%> (ø) ⬆️
lib/utils/addEntries.js 85.18% <100%> (-14.82%) ⬇️
lib/Server.js 92.73% <100%> (+0.22%) ⬆️
client-src/default/index.js 90.38% <81.25%> (-1.84%) ⬇️

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 303f4e9...5a6bd69. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 11, 2019

Codecov Report

Merging #2017 into master will increase coverage by 0.2%.
The diff coverage is 81.18%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2017     +/-   ##
=========================================
+ Coverage   92.77%   92.97%   +0.2%     
=========================================
  Files          29       30      +1     
  Lines        1149     1182     +33     
  Branches      327      332      +5     
=========================================
+ Hits         1066     1099     +33     
  Misses         79       79             
  Partials        4        4
Impacted Files Coverage Δ
lib/utils/guid.js 100% <100%> (ø)
lib/utils/updateCompiler.js 100% <100%> (ø) ⬆️
lib/servers/BaseServer.js 100% <100%> (ø) ⬆️
lib/servers/SockJSServer.js 96.29% <100%> (ø) ⬆️
lib/Server.js 92.76% <75.32%> (+0.25%) ⬆️

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 f611304...490c148. Read the comment docs.

Copy link
Member

@hiroppy hiroppy left a comment

Choose a reason for hiding this comment

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

need tests

Copy link
Member

@alexander-akait alexander-akait left a 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

const parsedQuery = querystring.parse(path);
compilerName = parsedQuery.compilerName;
}
})(__resourceQuery)
Copy link
Member

Choose a reason for hiding this comment

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

Move this to util

@@ -60,6 +60,7 @@ $(() => {
}
},
ok() {
console.log('OK!!');
Copy link
Member

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);
Copy link
Member

Choose a reason for hiding this comment

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

null?

return ((1 + Math.random()) * 0x10000 | 0).toString(16).substring(1);
}
return s4() + s4() + '-' + s4() + '-' + s4() + '-' + s4() + '-' + s4() + s4() + s4();
}
Copy link
Member

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

@orange4glace
Copy link
Author

orange4glace commented Jun 13, 2019

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.
@orange4glace
Copy link
Author

orange4glace commented Jun 13, 2019

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?

@alexander-akait
Copy link
Member

What is error(s) you got?

@orange4glace
Copy link
Author

Actually the error is much same with what Azure pipeline did. It seems error happens on Lint:prettier

Error log is here, and the complete log file is here

@alexander-akait
Copy link
Member

You can run tests using npm run test:only and avoid fixing linting issue right now, we can do this at last step of PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants