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.

Sign up for GitHub

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

Jump to bottom

Bugfix/console log #55

Merged
merged 18 commits into from
Mar 9, 2022
Merged

Bugfix/console log #55

merged 18 commits into from
Mar 9, 2022

Conversation

khackskjs
Copy link
Contributor

@khackskjs khackskjs commented Feb 9, 2022 •
edited
Loading

  1. now debug log always shows received false, I fixed it.
  2. to prevent wrong error log with webpack HMR, in my case vue.js web app.
    image

902seanryan and others added 17 commits January 11, 2021 15:04
@902seanryan
update version
631442b
@902seanryan
Merge branch 'dev' into release/3.2.1
5b6ccfe
@902seanryan
update changelog
6a8254b
@902seanryan
run full build
e100e57
@902seanryan
update changelog again
091deeb
@902seanryan
move codeowners into .github folder
b6b1a67
@902seanryan
Merge pull request SpringRoll#49 from SpringRoll/release/3.2.1
3145b2e 
Release/3.2.1
@dependabot
Bump ws from 7.4.2 to 7.4.6
672a244 
Bumps [ws](https://github.com/websockets/ws) from 7.4.2 to 7.4.6.
- [Release notes](https://github.com/websockets/ws/releases)
- [Commits](websockets/ws@7.4.2...7.4.6)

---
updated-dependencies:
- dependency-name: ws
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot
Bump lodash from 4.17.20 to 4.17.21
7796d54 
Bumps [lodash](https://github.com/lodash/lodash) from 4.17.20 to 4.17.21.
- [Release notes](https://github.com/lodash/lodash/releases)
- [Commits](lodash/lodash@4.17.20...4.17.21)

---
updated-dependencies:
- dependency-name: lodash
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot
Bump follow-redirects from 1.13.1 to 1.14.7
6b69f6d 
Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.13.1 to 1.14.7.
- [Release notes](https://github.com/follow-redirects/follow-redirects/releases)
- [Commits](follow-redirects/follow-redirects@v1.13.1...v1.14.7)

---
updated-dependencies:
- dependency-name: follow-redirects
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
@902seanryan
Merge pull request SpringRoll#52 from SpringRoll/dependabot/npm_and_y…
f6ed1a7 
…arn/follow-redirects-1.14.7

Bump follow-redirects from 1.13.1 to 1.14.7
@902seanryan
Merge pull request SpringRoll#51 from SpringRoll/dependabot/npm_and_y…
6c211d5 
…arn/lodash-4.17.21

Bump lodash from 4.17.20 to 4.17.21
@902seanryan
Merge pull request SpringRoll#50 from SpringRoll/dependabot/npm_and_y…
01c7f61 
…arn/ws-7.4.6

Bump ws from 7.4.2 to 7.4.6
@dependabot
Bump log4js from 6.3.0 to 6.4.0
361c298 
Bumps [log4js](https://github.com/log4js-node/log4js-node) from 6.3.0 to 6.4.0.
- [Release notes](https://github.com/log4js-node/log4js-node/releases)
- [Changelog](https://github.com/log4js-node/log4js-node/blob/master/CHANGELOG.md)
- [Commits](log4js-node/log4js-node@v6.3.0...v6.4.0)

---
updated-dependencies:
- dependency-name: log4js
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
@lukemelong
Merge pull request SpringRoll#53 from SpringRoll/dependabot/npm_and_y…
0d4eba3 
…arn/log4js-6.4.0

Bump log4js from 6.3.0 to 6.4.0
Remove webpack HMR log
17e7039
Fix received property of debug log
7ec4bb5
902seanryan
902seanryan requested changes Feb 11, 2022
View reviewed changes
src/Bellhop.js Outdated
@@ -115,7 +115,9 @@ export class Bellhop extends BellhopEventDispatcher {
// Check to see if the data was sent as a stringified json
if ('string' === typeof data) {
try {
if (data && !data.startsWith('webpack')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For now I'm going to request that this additional check be removed. See comment for more info

@902seanryan
Copy link
Contributor

@khackskjs
In regards to the webpack check. I think that check is just too specific to really require its inclusion. The error is technically correct since we're trying to parse a string rather than stringified JSON. Its just that the message is coming from Webpack not from within the Springroll ecosystem.
The error itself also shouldn't be halting any execution since its just a console.error call.
If the error message is a bigger problem, feel free to open up an issue and we can have a proper discussion on how best to handle it.

Thanks again for catching bugs and presenting fixes. Always appreciated!

@khackskjs
Copy link
Contributor Author

khackskjs commented Feb 13, 2022 •
edited
Loading

@902seanryan I understand what's your point and I agree that is' too specific case. as you said, it's just log without any collaption. so I suggest simply change log level with warn.
I believe error log is kind of unexpected situation or failed to work in business logic.
What do you think of it

@902seanryan
Copy link
Contributor

@khackskjs console.warn is a great compromise I think! @deycorinne if it sounds good to you I say we go for it.

@corinnejean
Copy link

@902seanryan sounds good to me. let's do it!

khackskjs
khackskjs commented Feb 18, 2022
View reviewed changes
src/Bellhop.js
@@ -307,7 +309,7 @@ export class Bellhop extends BellhopEventDispatcher {
*/
logDebugMessage(received = false, message) {
if (this.debug && typeof this.debug === 'function') {
this.debug({ isChild: this.isChild, received: false, message: message });
this.debug({ isChild: this.isChild, received, message: message });
Copy link
Contributor Author

@khackskjs khackskjs Feb 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@902seanryan @deycorinne Please check this line also.
false is supposed to be removed.
thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep! that one looks good! Just need the console.error changed to a console.warn and we can merge :)

902seanryan
902seanryan approved these changes Feb 18, 2022
View reviewed changes
Copy link
Contributor

@902seanryan 902seanryan left a comment

Choose a reason for hiding this comment

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

@khackskjs Sorry could you point this one to develop as well?

@khackskjs khackskjs changed the base branch from main to dev February 18, 2022 16:06
@khackskjs
Copy link
Contributor Author

Is there anything for me to do to merge blocked PR?
Reason why I ask you it's my first time to merge blocked PR that I made a PR

@902seanryan
Copy link
Contributor

Is there anything for me to do to merge blocked PR? Reason why I ask you it's my first time to merge blocked PR that I made a PR

Nope! Once its all good one of us will merge it in :)

@902seanryan 902seanryan merged commit 8394d8d into SpringRoll:dev Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@902seanryan 902seanryan 902seanryan approved these changes

@corinnejean corinnejean corinnejean approved these changes

@aberkie aberkie Awaiting requested review from aberkie

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@khackskjs @902seanryan @corinnejean @lukemelong