Suggestion: Make the fuzzy finder classes exposed/exported for other plugins to use

robobenklein commented 5 years ago

I've been working on some Atom plugins recently and I wanted a fuzzy-finder that I could insert into different places as wanted, however the code here didn't lend itself too openly to that.

Would you accept changes that made it easier for other plugins to gain access to classes such as ProjectView, FuzzyFinderView, and BufferView?

My current idea is that these could either be exported in the package's main module as-is, and potentially additional constructors to aid creating them to avoid the recent issue where the constructors now require the metricsReporter to function.

Adding these changes into the upstream would allow metrics reporting to be done in use cases outside the scope of this plugin, which may or may not be desirable.

Two examples I think are clear:

switch-header-source

https://github.com/dschwen/switch-header-source/issues/36

Using the require(module.path + /lib/X.js hack/workaround to gain access to the Fuzzy Finder, would be easier if this plugin provided access or even easier if this plugin provided a factory function for getting new instances of a properly constructed FuzzyFinderView. (with metrics linked, etc)

There's also a lot of other calls to this packages functions which would be massively simplified

CodeRibbon

A plugin I'm working on is using a fuzzy-finder as the background element for Panes which don't have any items in them. This allows users to keep the tree view closed more often and still easily see a list of recent / active files in the project.

In my case I extended some classes from this plugin (also using the require(modulepath + file) hack) to get rid of the use of metricsReporter since I didn't have access to that variable when constructing new FuzzyFinderViews.

Extending the FuzzyFinderView: https://github.com/utk-se/CodeRibbon/blob/9ad197cf915a75cd62cdc929e3a564863581896f/lib/fuzzyfinder/fuzzy-finder-view.js

Using a fuzzy finder instance per pane: https://github.com/utk-se/CodeRibbon/blob/9ad197cf915a75cd62cdc929e3a564863581896f/lib/code-ribbon-patch.js#L359-L369

others

There are probably some other packages I don't know about that also either are currently using classes from this plugin, or would greatly benefit from being able to use a fuzzy finder that's already well known and tested.

I'm willing to make a PR with whatever changes we'd think would best solve this problem - but I'm still unsure of a few things:

Should we expose/export the metricsReporter instance that fuzzy-finder creates, in order to allow use of the exported class constructors?
Should we provide generator/factory functions that return prebuilt and initialized instances of ProjectView, FuzzyFinderView, and/or BufferView?
If we do make factory functions for those classes, should I make createProjectView() and friends use those new factory functions or leave them as is?

50Wliu commented 5 years ago

It seems like what you're asking for could potentially be provided through services? Please let me know if you think that's the case.

robobenklein commented 5 years ago

I believe it could, although I don't know how I'd make such a clear API for it like for example status-bar has.

I can't immediately image what I'd pass via something like consumeFuzzyFinder, the only thing that I instinctively think of would be to pass those 4 views that people want access to, and maybe the metricsReporter.

Perhaps if I were to go with the factory route, consumeFuzzyFinder(factory_func) would give the consumer a function to call to generate new instances of ProjectView, FuzzyFinderView, etc?

function fuzzyFinderFactory ( type: string ) {
  switch (type) {
    return new instance of that type;
  }
}

// consumer has:
consumeFuzzyFinderFactory(ff_factory_function) {
  my_plugins_fuzzy_finder = ff_factory_function('fuzzy-finder');
}

I guess the reason I didn't immediately go this route is because I thought:

Switching construction of the finders to a factory seemed a bit overkill for this plugin
Other plugin developers (like me) probably want to be able to create their own classes that extend from FuzzyFinderView, etc

My preconception is that it's easier to use a package's a.packages.loadPackage('fuzzy-finder').mainModule.SomeBaseClass or const {FuzzyFinderView} = require('fuzzy-finder') to extend new classes from, although I'm not nearly as experienced as some other Atom developers are.

lee-dohm commented 5 years ago

Thanks for the feedback!

We don't feel that directly exposing the classes is the right way to go with this. Services are the mechanism that we created for Atom packages to depend on each other. This allows them to be versioned so that we don't have to worry about API mismatches. So we would be open to a well-written pull request that implements a service that other Atom packages could depend on.

Because this isn't something that we're going to work on ourselves, especially as described, we're going to go ahead and close this issue.

Thanks again for the great and detailed feedback!

atom / fuzzy-finder