Skip to content
This repository was archived by the owner on Oct 1, 2020. It is now read-only.

Conversation

@crdrost
Copy link

@crdrost crdrost commented Nov 21, 2017

... and stopping immediately on loops.

In order to support passthrough, on loops we still return whatever the dependent file was, even though we "cannot use it". Anything else would make the term "passthrough" very strange for what we're doing there. But this might not be a sane default for other compilers which get stuck in self-reference loops.

Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

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

In general seems like a good thing, just some performance / optimization things

* @private
*/
async compileUncached(filePath, hashInfo, compiler) {
async compileUncached(filePath, hashInfo, compiler, history=[]) {
Copy link
Member

Choose a reason for hiding this comment

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

To make this slightly faster we should just use a Set instead of an array

// Got something we can use in-browser, let's return it
return Object.assign(result, {dependentFiles});
} else if (history.indexOf(result.code) !== -1) {
d(`Compiler loop, I have seen this source before: ${JSON.stringify(result)}`);
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't log the compiler result, that's a lot of info to pipe to stdout

d(`Compiler loop, I have seen this source before: ${JSON.stringify(result)}`);
return Object.assign(result, {dependentFiles});
} else if (history.length > 30) {
d(`Runaway compilation: ${JSON.stringify({history: history, result: result})}`);
Copy link
Member

Choose a reason for hiding this comment

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

Same here, piping a lot of stuff to stdout actually takes a long time

@crdrost
Copy link
Author

crdrost commented Nov 21, 2017

Better?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants