Packager Refactoring

[alexlafroscia]

This is a distillation of some of the refactoring from #759 that I thought would be easier to review and talk about outside of the Vite packager I am working on there.

The gist is that rather than being an interface that packagers need to adhere to, it can instead be an abstract base class that packagers extend from. This keeps the build-time requirement of implementing an async build() method but gives us a way to move some common tasks into the base class for sharing.

I went ahead and kept the functionality that I moved to that base class in this PR. I found that while implementing the Vite packager, things like parsing the ember-app data out of the host app's package.json and copying files from the input to the output were things both packagers need to do, and I imagine other packages would likely need that as well!

The naming in this PR changes the semantics of some of the types a little, in a way that I think is a little more clear

Before

• Packager refers to a constructor for a Packager
• PackagerInstance refers to an instance of a Packager

After

• PackagerConstructor refers to a constructor for a Packager
• Packager refers to an instance of a Packager

I would have assumed that TypeScript has some kind of utility type for creating a type for a constructor of another class, but it surprisingly does not; the recommended approach is to use an interface in the way that this PR does.

Other Options

• We could also create two "tiers" of Packager base classes, if we wanted to; one that has the utilities and one that does not. I'm not sure that's really necessary, but it's definitely an option!

• Separate @embroider/packager packager -- this could take the Packager base class and move it to a separate package rather than having it be part of core. This really just comes down what should be part of core and what should not. I could see this going either way, personally, but left it in core for now (which is where the interface was originally defined).

  This would create a place for the html-entrypoint and stat-summary files, which have now been moved to core but are really just there so that both Packagers can use them. HTMLEntrypoint and the parsing it does is useful for both packagers, so moving it to a shared location would be really useful!

[ef4]

I think it's OK to have the shared code in core, but I would prefer utility functions over a base class. Not every packager implementation will want these utilities. Some packagers can natively speak HTML.

[alexlafroscia]

Is there a cost to a packager implementation having access to these utility as methods and just not using them?

If we want to go the utility-function route (which I think is totally fine) then I think it would be useful to have a separate package that contains the interface and utilities that a Packager-creator would likely need. That makes it more clear how to discover all those helpful bits, which I fear might get sort of "lost" if they're part of core with everything else. Would you be OK with a separate package as an organizational aid, from that perspective? Or would you prefer it all stays as part of core?

[ef4]

Is there a cost to a packager implementation having access to these utility as methods and just not using them?

This isn't primarily about performance cost. This is about inheritance-as-framework-API being a pretty fraught area of API design.

That makes it more clear how to discover all those helpful bits, which I fear might get sort of "lost" if they're part of core with everything else.

In practice, people are going to write packagers by looking at what the existing packagers do. So I don't find this argument about discoverability compelling.

Also consider that if we make a separate package, then we'll need to manage API versioning between it and core. @embroider/webpack has a peerDependency instead of dependency on @embroider/core on purpose -- it means you can't accidentally use a version that doesn't match the app's version. If we made a separate utility package, it would need to similarly be a peerDep, but that means it's yet another thing apps need to manage, and every time an app is updating deps the author will need to think about what versions to pick that work together. It's a real cost that I wouldn't want to impose without a good reason.

[alexlafroscia]

This isn't primarily about performance cost. This is about inheritance-as-framework-API being a pretty fraught area of API design.

That makes sense to me! I wasn't even thinking about performance when I asked, but more about other types of costs that might guide the design choice here. What you've described here sounds like a cost to avoid to me!

The bit about keeping it in core makes sense to me too -- I'll take another stab at these changes while keeping they interface design and utility functions.

Do you have a preference on the Packager+PackerInstance vs PackagerConstructor+Packager naming choice?

[ef4]

I like PackagerConstructor+Packager just because that's the norm for typescript (the name of the class is the type of the instance).

[alexlafroscia]

I started this over again and simplified the changes

• Packager and PackagerConstructor are just interfaces now
• getAppMeta is extracted as a utility instead, and is the only "shared utility" that is extracted for now
• Tests added for the getAppMeta utility

I decided not to bring over the file-copying behavior yet and instead just copy that manually into the Vite packager (at least, for now).