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

Logger singleton #462

Merged
merged 2 commits into from
Jan 31, 2018
Merged

Logger singleton #462

merged 2 commits into from
Jan 31, 2018

Conversation

DeMoorJasper
Copy link
Member

Converts Logger into a singleton, so everything outside of bundler can use it.
Also created IPC for logger so that assets can send messages to the workerfarm, instead of overloading logger singleton.

After seeing #207 was too big of change and failing times and times again to fix the glitches with status i thought it be a better idea to just extract the singleton part of that PR and make a fresh clean new PR, without the conflicts, ...

@codecov-io
Copy link

codecov-io commented Jan 31, 2018

Codecov Report

Merging #462 into master will decrease coverage by 0.06%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #462      +/-   ##
==========================================
- Coverage   88.99%   88.92%   -0.07%     
==========================================
  Files          62       62              
  Lines        1971     1986      +15     
==========================================
+ Hits         1754     1766      +12     
- Misses        217      220       +3
Impacted Files Coverage Δ
src/transforms/uglify.js 94.44% <100%> (+0.32%) ⬆️
src/utils/generateCertificate.js 79.41% <100%> (+0.62%) ⬆️
src/FSCache.js 15.68% <50%> (+1.68%) ⬆️
src/HMRServer.js 87.87% <50%> (+0.37%) ⬆️
src/WorkerFarm.js 91.22% <66.66%> (-3.22%) ⬇️
src/Server.js 90.9% <66.66%> (+0.21%) ⬆️
src/Logger.js 43.54% <75%> (+5.36%) ⬆️
src/Bundler.js 88.84% <90%> (ø) ⬆️

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 1d49e47...ed41f02. Read the comment docs.

@devongovett
Copy link
Member

Sorry for the delay getting this in. I just cleaned up the IPC a little, so we aren't adding logging things to the Asset class. Instead, a proxy class is exported by the Logger in workers that sends IPC messages. That way you can use the normal logger "singleton" as usual but the messages will actually get sent to the main process. Seems to work well!

@devongovett devongovett merged commit 7518805 into parcel-bundler:master Jan 31, 2018
@DeMoorJasper DeMoorJasper deleted the feature/logger-singleton branch January 31, 2018 08:32
@brandon93s
Copy link
Contributor

@devongovett - clever idea with the proxy logger! 👍

@ioss
Copy link

ioss commented Mar 5, 2018

@devongovett this is a breaking change (not sure if this project follows semver?).

I suggest to put the logger back as a member to the bundler; so plugins that use bundler.logger do not break.
Also the logger should be available as Bundler.Logger for new/updated plugins, so that there is no need for a plugin developer to do the src vs. lib check, which if done wrong (or if parcel should change it's check) could lead to working with two "Singletons" ;)

devongovett pushed a commit that referenced this pull request Oct 15, 2018
* initial logger singleton

* Clean up Logger IPC
devongovett pushed a commit that referenced this pull request Oct 15, 2018
* initial logger singleton

* Clean up Logger IPC
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.

5 participants