Skip to content
This repository was archived by the owner on Sep 16, 2022. It is now read-only.

Can we remove "placeholder" files? #864

Closed
matanlurey opened this issue Feb 9, 2018 · 14 comments
Closed

Can we remove "placeholder" files? #864

matanlurey opened this issue Feb 9, 2018 · 14 comments

Comments

@matanlurey
Copy link
Contributor

matanlurey commented Feb 9, 2018

We have the concept of a .ng_placeholder file, which means, roughly:

For {file}.ng_placeholder a {file}.template.dart will exist in a future phase.

This is because while we can check the file system for dependencies, our own current "target" (terminology varies based on bazel or build_runner) may not have wrote certain files yet. Imagine the following:

package:foo
  /lib
    a.dart
    b.dart

... and imagine a.dart looks like this:

import 'b.dart';

We need to be able to generate, in a.template.dart:

import 'b.template.dart' as _ref0;

void initReflector() {
  _ref0.initReflector();
}

Ideas

In bazel, pass generate_for / inpus as a command-line argument:

$ generator --input-list="a.dart,b.darrt"

In build_runner ...

I'm not sure. Maybe we have to continue to use placeholder files here.

/cc @jakemac53 @natebosch @leonsenft

@natebosch
Copy link
Contributor

What are the downsides to having it?

Are we OK keeping the file as part of the build if we can remove it from the deployed output? dart-lang/build#189

@matanlurey
Copy link
Contributor Author

What are the downsides to having it?

Confusing our engineers when we remember it exists every 3-6 months 😆

Are we OK keeping the file as part of the build if we can remove it from the deployed output?

This is certainly better, but not perfect.

@leonsenft
Copy link
Contributor

@natebosch I don't understand why the builder can't tell us that file (will) exists? Aren't all the outputs declared ahead of time?

@jakemac53
Copy link
Contributor

jakemac53 commented Feb 9, 2018

@leonsenft we can know that it might exist, but not that it will exist.

Also exposing an api for this would be tricky, because your builder now has to have a dependency on the fact that there is a declared output for that node, but right now you are only allowed to have inputs from phases earlier than your own. We would have to add some sort of special node type or something to represent this type of dependency, but it should be doable.

@natebosch
Copy link
Contributor

In bazel we could know that it will exist. (since we have default content for it). We'd still need some brand new config to express our desire to pretend it exists without actually writing it.

@leonsenft
Copy link
Contributor

@jacob314 Why is that? Excuse my complete lack of knowledge of the package:build* family implementation, but don't you declare your inputs/outputs in the build.yaml?

So if I declare that for each *.dart file, a corresponding *.template.dart file exists, can we not first resolve which outputs will exist, before running the builders on the inputs?

Or can a builder opt not to generate the corresponding output? Or do we not walk the project tree to collect all inputs before running the builders?

@jakemac53
Copy link
Contributor

jakemac53 commented Feb 9, 2018

@leonsenft Yes a builder can opt to not output a file it declares in build_runner, so we can avoid creating a bunch of empty files like we do in bazel.

That builder still "owns" that file though, so nobody else can output it even if they decide not to.

@leonsenft
Copy link
Contributor

Understood, thanks!

@matanlurey
Copy link
Contributor Author

@jakemac53 How would you feel about at least removing these files in Bazel-mode?

@jakemac53
Copy link
Contributor

@matanlurey We could potentially remove it in both modes if we gave the builder access to the generateFor argument in the BuilderOptions?

Angular could know what it was asked to build, and from that make safe assumptions about what it will output. Same thing could work in bazel and build_runner.

We also already support proper invalidation based on the BuilderOptions.

@matanlurey
Copy link
Contributor Author

I'll leave the details up to you, but if you're OK with it would be nice to remove this complexity.

@leonsenft
Copy link
Contributor

@jakemac53 What is the distinction with that and what I asked? From your last reply I got the impression the builder could still potentially not produce that file?

@natebosch
Copy link
Contributor

tl;dr; - synced with Jake offline and letting you read generateFor isn't really feasible. We think that the placeholder file is the best solution to this problem and we hope that finding a solution for transient files will mitigate most of the negatives, but I think we'll have to live with the confusing pattern for the same reason we need to live with other workarounds to the limitations of build

longer replies:

From your last reply I got the impression the builder could still potentially not produce that file

In build_runner you're allowed to not produce a file. In bazel there are 2 sets of build extensions in play. The ones in Dart code are a max - you can't produce anything not specified but may choose not to produce some. The ones in skylark are a minimum - you must produce all the extensions you specified (or provide default content and we'll produce them for you).

We could potentially remove it in both modes if we gave the builder access to the generateFor

This, unfortunately, isn't sufficient to make the decision. In build_runner we may choose to interleave the placeholder build step across multiple packages if there is an import cycle. If we give you the generateFor from the action that is running it would not be sufficient to answer the question you need answered for an import from another package in a cycle. Further, the actual primary inputs you get are determined by both the target sources as well as the builder generate_for, and each has include and exclude globs so you'd need to run some complex checks using a lot of information from the user's setup.

If we did work out these problems we end up with a lot of code to support properly invalidating outputs when files are added or removed, and could end up calling builds more often than necessary - we can prune more intelligently with the placeholder file.

@matanlurey
Copy link
Contributor Author

Thanks! I'm going to mark this as "not planned" for now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants