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

fix(compartment-mapper): add support for alternatives in exports defnitions #1134

Merged
merged 2 commits into from Apr 5, 2022

Conversation

naugtur
Copy link
Member

@naugtur naugtur commented Mar 30, 2022

There's no point in iterating the array, because in the top 1000+ packages with exports field defined as an object, all examples using an array define the same export twice in the array - once as an object with default/import alternatives and another as string.

We support both ways, so taking [0] is enough of an alternative choice algorithm.

Edit: Iterating the array now, for correctness ;)

@naugtur naugtur requested a review from kriskowal March 30, 2022 09:21
@naugtur naugtur force-pushed the naugtur-require-reaching-nested-files branch from a9d5f01 to 2a2bf1c Compare March 31, 2022 08:38
// Find one that produces non-empty result, discard result and use again
exports.find(ex => !interpretExports(name, ex, tags).next().done),
tags,
);
Copy link
Member Author

Choose a reason for hiding this comment

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

Alternative implementation that doesn't perform one unnecessary operation just feels clunky

    let sample,
      alternative,
      i = 0;
    do {
      alternative = interpretExports(name, exports[i], tags);
      sample = alternative.next();
      i++;
    } while (sample.done);

    yield sample.value;
    yield* alternative;

This is not a heavy operation, so I decided to go with spawning 2 copies of the successful generator.

Copy link
Member

Choose a reason for hiding this comment

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

It took me a moment to read, but I agree it’s better than the alternative you propose. I had imagined something like:

for (const section of exports) {
  const results = [...interpretExports(name, section, tags)];
  if (results.length > 0) {
    yield* results;
    break;
  }
}

I’m convinced that your solution is equivalent.

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'll replace with this to avoid having to look at .next().done. Thanks.

@naugtur naugtur changed the title fix(compartment-mapper): add support for alternatives in exports defnitions by always picking the first alternative fix(compartment-mapper): add support for alternatives in exports defnitions Mar 31, 2022
Comment on lines +12 to +14
{
"deno": "./this-gets-skipped"
},
Copy link
Member

Choose a reason for hiding this comment

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

// Find one that produces non-empty result, discard result and use again
exports.find(ex => !interpretExports(name, ex, tags).next().done),
tags,
);
Copy link
Member

Choose a reason for hiding this comment

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

It took me a moment to read, but I agree it’s better than the alternative you propose. I had imagined something like:

for (const section of exports) {
  const results = [...interpretExports(name, section, tags)];
  if (results.length > 0) {
    yield* results;
    break;
  }
}

I’m convinced that your solution is equivalent.

@naugtur naugtur force-pushed the naugtur-require-reaching-nested-files branch from d37fce7 to 01a9d04 Compare April 5, 2022 08:36
@naugtur naugtur merged commit 6663f25 into master Apr 5, 2022
@naugtur naugtur deleted the naugtur-require-reaching-nested-files branch April 5, 2022 08:50
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.

None yet

2 participants