Skip to content

Glob nodes #1722

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

Merged
merged 9 commits into from
Aug 7, 2018
Merged

Glob nodes #1722

merged 9 commits into from
Aug 7, 2018

Conversation

jakemac53
Copy link
Contributor

@jakemac53 jakemac53 commented Aug 6, 2018

Fixes #1702

  • Adds a new node type GlobAssetNode, and a new interface (NodeWithInputs) for nodes that have some inputs and an invalidation state.
  • Results of the glob are cached on the node, and re-used across the same phase.
  • When using findAssets, the glob node gets as inputs all the available nodes, and is invalidated just like any other generated node when those inputs are invalidated.
  • On new/deleted sources we still need extra logic, but now it only iterates the glob nodes to check for globs which is more efficient than before in the case that a lot of nodes in the same phase performed the same globs.

@jakemac53 jakemac53 requested a review from natebosch August 6, 2018 20:56
@googlebot googlebot added the cla: yes Google is happy with the PR contributors label Aug 6, 2018

/// A node representing a glob ran from a builder.
///
/// The [id] must always be unique to a given package, phase, and glob
Copy link
Member

Choose a reason for hiding this comment

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

should this constructor generate the id from package/phase/pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it could but we need to generate these outside of the constructor to check for their existence in the graph as well

Copy link
Member

Choose a reason for hiding this comment

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

maybe add it as a static method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return node.isReadable && node.isValidInput;
}

FutureOr<dynamic> _ensureAssetIsBuilt(AssetNode node) {
Copy link
Member

Choose a reason for hiding this comment

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

should this be FutureOr<void>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

doAfter(_updateGlobNodeIfNecessary(globNode), (_) => globNode));
}

FutureOr<Null> _updateGlobNodeIfNecessary(GlobAssetNode globNode) {
Copy link
Member

Choose a reason for hiding this comment

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

FutureOr<void> (we'll need to start fixing this elsewhere, but shouldn't introduce new cases)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}
globNode.results = actualMatches;

globNode.state = NodeState.upToDate;
Copy link
Member

Choose a reason for hiding this comment

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

cascade operator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@jakemac53 jakemac53 merged commit 9545526 into master Aug 7, 2018
@jakemac53 jakemac53 deleted the glob-nodes branch August 7, 2018 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google is happy with the PR contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants