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

Add experimental utils #9441

Merged
merged 16 commits into from
Mar 31, 2021
Merged

Add experimental utils #9441

merged 16 commits into from
Mar 31, 2021

Conversation

ddbeck
Copy link
Collaborator

@ddbeck ddbeck commented Mar 12, 2021

This PR adds some experimental utilities for traversing compat data. These are not intended to be included in the package and won't (at least for now) constitute a public API.

It's kinda big and it's got some rough edges. I'll do a self-review to highlight a few things.

Related to #6585

Originally from: https://github.com/ddbeck/bcd-utils

@github-actions github-actions bot added dependencies Pull requests that update a dependency package or file. infra Infrastructure issues (npm, GitHub Actions, releases) of this project labels Mar 12, 2021
const { isBrowser, descendantKeys, joinPath } = require('./walkingUtils');
const query = require('./query');

function* lowLevelWalk(data = bcd, path, depth = Infinity) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why two walking generators? walk() wraps lowLevelWalk() but only yields __compat: { … } objects. lowLevelWalk() is the basis for other sorts of interesting walks, such as walking non-__compat namespaces or walking browsers, which have not been implemented yet.

});
it('visits every point in the tree', function () {
const paths = Array.from(lowLevelWalk()).map(step => step.path);
assert.ok(paths.length > 13000);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Strictly speaking, this doesn't really test this function—just that its output is reasonable—but I feel like the tests on walk() do the heavy lifting here.

);
});

it('should yield every feature by default', function () {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test (and the similar test of visit) are rather slow (>300ms and >50ms on my machine), which Mocha highlights. I think it's mostly the overhead of Mocha, but we could restrict this to some subtree if we want to speed things up.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could also have a unchanging set of BCD-like JSON files for testing only, which could exercise things that are uncommon in BCD or likely to change over time. That would probably be blazing fast to test with.

One might still have one or two smoke tests involving the real data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Testing with a unchanging set will be a lot easier now that we've got the consistent data parameter.

Is this something I should come up with as part of this PR?

ddbeck added a commit to ddbeck/bcd-utils that referenced this pull request Mar 12, 2021
@ddbeck ddbeck marked this pull request as ready for review March 12, 2021 20:23
Copy link
Contributor

@foolip foolip left a comment

Choose a reason for hiding this comment

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

I've tried using walk('api') to answer a quick question I had with success.

Base automatically changed from master to main March 24, 2021 12:54
Copy link
Contributor

@foolip foolip left a comment

Choose a reason for hiding this comment

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

Let's do it!!

@github-actions github-actions bot added the docs Issues or pull requests regarding the documentation of this project. label Mar 31, 2021
@ddbeck
Copy link
Collaborator Author

ddbeck commented Mar 31, 2021

Last week, I talked with Florian and he suggested that I document the experimental status somewhere. I've done that by adding a README file to the utils folder. With that satisfied, I'm going to merge this.

Thank you for the excellent reviewing here, @foolip!

@ddbeck ddbeck merged commit 44f3b7e into mdn:main Mar 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency package or file. docs Issues or pull requests regarding the documentation of this project. infra Infrastructure issues (npm, GitHub Actions, releases) of this project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants