add some defensive programming to fix cases where babel is run without a filename

[mansona]

I got a report that generators aren't working on the new app blueprint so I added a test to make sure we don't miss issues like this again: embroider-build/app-blueprint#74

Essentially I ran the blueprint locally and tracked down these two places in this PR where we could do with a bit of defensive programming. The real issue is that ember-cli really shouldn't be initialising our babel config when running ember generate but that's not something we can fix in every version of ember-cli out there, it's better to be a tiny bit defensive 👍

[ef4]

I also saw this failure recently, it was due to ember-composable-helpers. ember-cli-babel itself has been ignoring a babel config for quite a long time.

I don't think we should hide these particular exceptions. Making them not throw doesn't make them safe. Also, I'm not OK with making the types lie like this.

[mansona]

Turns out it's https://github.com/ember-cli/ember-cli/blob/91175c30e75c1500162da175a50e00bd7f449b99/lib/models/blueprint.js#L531-L564 that's the problem (because we're seeing this in generators)

and it looks like I've tried to fix this already but no response 🫠 cafreeman/remove-types#3

I still think it's probably a good idea to be defensive, even though I agree that we don't want to hide legit errors.

I'm not OK with making the types lie like this.

But we're already in a place where types are lying, you can't assume that we always have a filename because babel can be run on a string representation of the code 🤷 so who is lying about the types already?

[NullVoxPopuli]

If anyone finds this thread and is using old compostable helpers, i have a fork: https://github.com/NullVoxPopuli/ember-composable-helpers

Which has no build behavior (but does have instructions for aliasing old packages)

[ef4]

But we're already in a place where types are lying, you can't assume that we always have a filename because babel can be run on a string representation of the code 🤷 so who is lying about the types already?

This is true so let me rephrase my point: we should stop the inconsistency at the boundary of our system and not let it propagate. It just moves the failures around. Everybody who calls cleanURL is assuming they can trust that the result isn't undefined. Which of them will throw sometimes if it's undefined?

More subtly, callers of ownerOfFile are trusting that if the file has an owner they're going to get it. Silently not getting it because babel is misconfigured could lead to confusing problems.

In the specific case of the generators, it's probably very bad if the generator surprisingly executes our babel config. People would have no way of knowing that the code that got generated for them actually contains unintended build output. If somebody makes a blueprint that contains use of embroider macros, the macros would run during generate and no longer be present in the user's newly generated files!

So no, I don't think we should be letting our babel config half-work when it's accidentally sucked into the wrong contexts. It could do bad things in those contexts. Fork remove-types if upstream is unresponsive.

[ef4]

If we want this problem to be less confusing, I would be in favor of making our babel plugins throw early if filename is undefined, with a message explicitly about that.

[ef4]

As an alternative workaround, if we only want to patch the callsite of remove-types and not remove-types itself, we can wrap it in a chdir to ensure that it won't see any of the app's babel config when it runs.

[mansona]

Is it just me or is this default behaviour of babel auto-discovering babel configs a totally incorrect default!? 🙈

Your suggestions seem sound, if I were to choose I would probably go with a fork since it gives us the control we need over the dependency. The issue I have with either approach though is that we need to patch this in ember-cli 😫 do we just patch our LTS versions and make sure we document that people could override the bad package if they are hitting this on any other version?

[ef4]

We should probably figure out what the realistic upgrade paths are that we want to support. It will depend on which exact releases people find hard to upgrade across.

It has been standard advice for people to upgrade from LTS to LTS, so that would support the idea of us backporting fixes to every LTS. But, for example, if we know there are no major gotchas between 4.0.0 and 4.12.0, we could tell people to jump directly from 3.28 to 4.12.4, and only worry about supporting those.