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

Packager Refactoring #765

Merged
merged 6 commits into from
May 19, 2021
Merged

Conversation

alexlafroscia
Copy link
Contributor

@alexlafroscia alexlafroscia commented Apr 15, 2021

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
Copy link
Contributor

ef4 commented Apr 15, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

ef4 commented Apr 19, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

ef4 commented Apr 19, 2021

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

@alexlafroscia alexlafroscia changed the title Extract Packager Base Class Packager Refactoring Apr 19, 2021
@alexlafroscia
Copy link
Contributor Author

alexlafroscia commented Apr 20, 2021

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).

ef4 added 2 commits May 19, 2021 11:29
"Stat" is a webpack-specific concept, so I'd prefer to deemphasize it in the naming and organization and keep this as an implementation detail of HTMLEntrypoint.
@ef4 ef4 merged commit 96d4c29 into embroider-build:master May 19, 2021
@ef4 ef4 added the internal label May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants