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

[WIP] Logger improvements #207

Closed
wants to merge 52 commits into from
Closed

[WIP] Logger improvements #207

wants to merge 52 commits into from

Conversation

DeMoorJasper
Copy link
Member

@DeMoorJasper DeMoorJasper commented Dec 11, 2017

  • Change logger into a global accessible Singleton
  • Improve message outputting to not overwrite and glitch
  • Add the ability to make any message persistent
  • Full control over the console

@DeMoorJasper DeMoorJasper changed the title Feature/logger enhancements Logger improvements Dec 11, 2017
@albinotonnina
Copy link
Contributor

I see you found the solution for your logger issue in #157 👍

@yeskunall
Copy link
Contributor

yeskunall commented Dec 11, 2017

+1 for converting Logger into a Singleton.

src/Bundler.js Outdated
this.logger.warn(err);
setTimeout(() => {
logger.warn(err);
}, 500);
Copy link
Contributor

Choose a reason for hiding this comment

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

Whats the 0.5 second delay for?

Copy link
Member Author

Choose a reason for hiding this comment

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

So that the warning doesn't get removed when the builder actually starts and does logger.clear()

Copy link
Member Author

Choose a reason for hiding this comment

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

Nvm this is no longer necessary in the latest version...

Copy link
Contributor

@brandon93s brandon93s left a comment

Choose a reason for hiding this comment

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

😬 merge conflicts.

Seems like a worthwhile resolve / merge.

@DeMoorJasper
Copy link
Member Author

All conflicts should be fixed now @brandon93s

@devongovett
Copy link
Member

Thanks @DeMoorJasper! I tried this out locally. A couple things:

  • I like the idea of making it a global singleton. The problem is that it isn't really a singleton since different instances will be accessed by each worker. This will cause the history to be overwritten by each worker. To fix this, I think we should use IPC to send messages from worker instances to the master, which will actually do the logging. Should be fairly easy.
  • This seems to cause the logger to take over the whole terminal rather than write inline. e.g. if you had other history in your terminal prior to running parcel, it would be overwritten when parcel starts. I'd rather if it remained inline so that we only ever overwrite other messages from Parcel itself and not other commands.

@DeMoorJasper
Copy link
Member Author

DeMoorJasper commented Dec 26, 2017

@devongovett i removed the singleton pattern, just wondering how u see implementing the IPC, as far as i've thought about it i think it would overwrite messages by parcel or cause the glitches i tried to fix in the first place with keeping an array with all messages (overwriting status every time and having full control over the output on clear and rewrites)
Also wondering how to do this in a streamlined way, with assets not being processed using the workerfarm, because these don't have the process.send

@DeMoorJasper DeMoorJasper changed the title Logger improvements [WIP] Logger improvements Dec 27, 2017
@devongovett
Copy link
Member

Sorry I wasn't clear. I DID like the singleton pattern you had, we just need to add something so that the instances in workers don't overwrite each other.

Maybe something like this could work? The child would intercept writes to the logger and send them to the master.

write(message) {
  if (process.send) {
    // we're in a worker
    process.send({type: 'log', message});
  } else {
    // do actual write here
  }
}

process.on('message', msg => {
  if (msg.type === 'log') {
    Logger.write(msg.message);
  }
});

src/Logger.js Outdated
let hasStatusLine = this.statusLine != null;
if (!hasStatusLine) {
this.statusLine = this.lines;
handleMessage(options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

handleMessage could be simplified as:

handleMessage(options) {
  this[options.method](...options.args);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for this, changed it

src/Logger.js Outdated
}

module.exports = Logger;
module.exports = getLogger();
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably export Logger rather than an instance, so that it's possible to inherit from Logger or extend it with mixins.

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'm not sure if this is really necessary but i exported both in new improvements

@DeMoorJasper DeMoorJasper mentioned this pull request Jan 1, 2018
@DeMoorJasper
Copy link
Member Author

DeMoorJasper commented Jan 1, 2018

Closing this in favor of #462 once I figure out how to make the statuses not glitch i'll just open up a new PR.

@DeMoorJasper DeMoorJasper deleted the feature/logger-enhancements branch January 1, 2018 13:03
@pmurias
Copy link

pmurias commented May 23, 2018

A problem here on xterm + bash + Ubuntu.
I have to write to pipe to cat to avoid this horrible error message maiming.

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.

7 participants