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

Feature Request: Revise index.export() to return an Array of Promises #274

Closed
WesleyMConner opened this issue Oct 6, 2021 · 12 comments
Closed
Assignees
Labels
enhancement New feature or request

Comments

@WesleyMConner
Copy link

Currently

  • The function index.export( function(key, data) {..} ), returns a single value - the boolean true.
  • The registered callback function is invoked four times, once each for keys reg, cfg, map and ctx.

Requested Improvement

-Adjust index.export() to return an array of promises - i.e., one promise for each prospective callback invocation. [Alternately, return an iterable suitable for a Promise.All() construct.]

Benefits

  • The proposed would facilitate tracking (e.g., blocking on) the resulting N callbacks.
  • Going forward, Flexsearch could adjust the number of callback invocations with less (if any) client impact.
  • Errors (rejections) could be handled within the Promise framework.

Example

Simulate an exception in the callback function.

myErrantCallback (k, v) {
  // Convert k to kx for https://github.com/nextapps-de/flexsearch/issues/273
  let kx = k;
  if (k.lastIndexOf('.') > 0) {
    kx = k.substring(k.lastIndexOf('.') + 1);
  }
  // Throw a random exception 15% of the time.
  if (Math.floor(Math.random() * 100) < 15) {
    throw new Error(`k: ${k}, kx: ${kx}, v: ${v}`);
  } else {
    console.log(`k: ${k}, kx: ${kx}, v: ${v}`);
  }
}

The current export() facility, requires clients to anticipate and track the number of callback invocations AND deal with any exceptions that arise. The proposed facility simplifies these tasks.

Promise.All(index.improvedExport( function(key, data) {..} )
  .then(...)     // Take an action after all N Promises are fulfilled.
  .catch(...)    // Centrally manage exceptions.
@peterbe
Copy link

peterbe commented Oct 18, 2021

If I had this, I could do something like:

const bits = await Promise.all(index.export)
await fs.writeFile(bits, JSON.stringify(bits))

@rpsirois
Copy link

rpsirois commented Apr 4, 2022

#273
#274
#290
#299

function exportIndex(flexSearchIndex) {
  // https://github.com/nextapps-de/flexsearch/issues/299
  // https://github.com/nextapps-de/flexsearch/issues/274
  return new Promise((res, rej) => {
    let pkg = [];
    const expected = new Set([
      "reg",
      "reg.cfg",
      "reg.cfg.map",
      "reg.cfg.map.ctx"
    ]);
    const received = new Set();

    const setsEq = (a, b) => {
      if (a.size != b.size) {
        return false;
      }

      return Array.from(a).every(el => b.has(el));
    };

    flexSearchIndex.export((key, data) => {
      // https://github.com/nextapps-de/flexsearch/issues/290
      // https://github.com/nextapps-de/flexsearch/issues/273
      pkg.push([
        key
          .toString()
          .split(".")
          .pop(),
        data
      ]);
      received.add(key);

      if (setsEq(expected, received)) {
        res(JSON.stringify(pkg));
      }
    });
  });
}

@mmm8955405
Copy link

I don't need you to do anything. I just want to know how to deal with the data exported.

@mmm8955405
Copy link

If you return a promise, you limit the grammar. It's very painful for people who don't need grammar candy. How should we handle the exported and imported data. I found that there is still undefined content. Why can't we use nodejs streaming?

@rpsirois
Copy link

rpsirois commented Apr 9, 2022

We're working on a solution internally at my company. Someone will pull request when it makes it on the top of the to-do stack. In the short term this worked for me.

What do you mean by grammar? Ideally the function would return a promise but also support callback style (well, without it being called for each key separately).

@jdddog
Copy link

jdddog commented Apr 28, 2022

@rpsirois thanks for sharing the function for exporting an index, it is very useful. I just wanted to double check if you intended to release it under the same license (Apache 2.0) as flexsearch?

@rpsirois
Copy link

There's no license it's just some random code posted on the internet. If we do a pull request it will fall under the FlexSearch license since the code will be incorporated into their project.

@danielvy
Copy link

danielvy commented Jun 2, 2022

I don't think that export() needs to return an array of promises. If you need an array of promises you can just create one and add the created promises in the callbacks, as seen in the example provided by @rpsirois . But you probably never need this.

What doesn't make sense is that export() is an asynchronous function by nature that invokes a callback and yet it doesn't provide any way to know when it's done invoking callbacks for all keys. So users need to jump through hoops and compare the keys provided to callbacks with some list of possible keys.

What's needed is that export() should return a single promise when it's done, i.e. finished calling all callbacks, and, if needed, waiting for callbacks that return promises. Then resolve.

So you can simply write:

await index.export();

By the way, the Typescript definitions incorrectly (for the actual code) type export() as returning Promise<void>, which was very confusing.

@ts-thomas ts-thomas self-assigned this Oct 2, 2022
@ts-thomas ts-thomas added the enhancement New feature or request label Oct 2, 2022
@ts-thomas
Copy link
Contributor

This is now fixed in v0.7.23
Further improvements to provide Promise.all() compatible export ist coming in next version.

@WesleyMConner
Copy link
Author

This is great news!

I don't see a v0.7.23 tag or npmjs.com release.

What's the recommended way to pull and benefit from the latest stable code?

ts-thomas added a commit that referenced this issue Oct 3, 2022
@rpsirois
Copy link

rpsirois commented Oct 3, 2022

@ts-thomas Thanks for all your hard work on this!

@WesleyMConner
Copy link
Author

I see 0.7.3 and 0.7.31 now. Perfect timing as I'm in this code today.

Very much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants