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

Using input/output facade #187

Closed
wants to merge 1 commit into from

Conversation

SparshithNR
Copy link
Contributor

Using input/output facade inside the broccoli-persistent filter.

index.js Outdated Show resolved Hide resolved
@SparshithNR SparshithNR mentioned this pull request Dec 19, 2019
1 task
lib/dependencies.js Outdated Show resolved Hide resolved
* Passing custom fs to dependencies so that we can use fs facade.
* Removing ts-ignores
* deserialize must be provided with customFS
* to keep the encapsulations.
* @type {typeof fs}
*/
this.fs = options.fs || fs;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although functionally correct, this fallback should most likely be removed. Under what circumstances should we allow native fs to be used now?

My feeling is that, allowing for this fallback may hide bugs. Specifically bugs where some future PR accidentally causes options.fs to be falsy causing an erroneous fallback to fs. The resulting issues would likely be hard to diagnose.

Since this is an internal private object, I would prefer if we make it exactly what we need, and make options.fs mandatory, with no graceful fallback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was done to provide backward compatibility for those who extended the Filter class.

Copy link
Contributor Author

@SparshithNR SparshithNR Dec 20, 2019

Choose a reason for hiding this comment

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

I am not sure if there is a real use case for this.
an example will be

class awk extends Filter {
build () {
// we can won't break this implementation.
let srcDir = this.inputPaths[0];
 this.dependencies = this.processor.initialDependencies(srcDir);
 // .... do build related stuffs.
}
processString(content) {
 return content;
}
}

The current implementation need to make a slight change where I assume srcDirs in new Dependency(srcDirs) always receives array, I have to make a change to convert if the argument is not an array change to an array.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@SparshithNR that would suggest to me, that we have just changed the signature of initializeDependencies in a breaking way, that is unexpected. Users having to remember to pass this.input if they invoke the initialDependencies stuff seems very unfortunate.

Can we pass the needed details to the processor when it is initialized? That would ensure no breaking changes occurs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately no, we cannot pass it while initializing the processor. The processor gets initialized at the constructor and this.input is accessible only at the build time.

@SparshithNR
Copy link
Contributor Author

Closing this as #191 addressed this.

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.

2 participants