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

one tap, multiple plugin sets #945

Open
3 tasks
isaacs opened this issue Oct 8, 2023 · 3 comments
Open
3 tasks

one tap, multiple plugin sets #945

isaacs opened this issue Oct 8, 2023 · 3 comments

Comments

@isaacs
Copy link
Member

isaacs commented Oct 8, 2023

While the issues in #940 can be addressed, what this is really highlighting is that it'd be nice to be able to have a single tap install, but different sets of plugins in different folders, which is going to take a more thoughtful refactoring.

current state

The tap package is a very thin wrapper around the TAP object exported by @tapjs/core, and an executable runner that calls the @tapjs/run executable.

@tapjs/core pulls in the built Test class from @tapjs/test, and the TAP class is inherited from that.

Right now, there's exactly one @tapjs/test, which is where all the plugins get built, and because of that, there can only ever be one set of plugins for a given tap install. We can be smarter about knowing where to install/build plugins, which tap to load in the runner, etc.

In order to have a built @tapjs/test per project, the Test class will have to be built in that project folder somewhere. So the challenge is to make sure that it is:

  1. as small as possible
  2. the thing that gets loaded when the test does require('tap') or import 'tap'
  3. statically analyzable by typescript, or else we lose the benefits of strict static typing coming from all plugins automatically.

The tap package is small enough that it's fine to put another copy in the project. But @tapjs/core is pretty big, as is the runner, reporter, etc. So any approach would have to keep those things out.

For any approach, we can skip it if the tap that is found by the project has the same plugin signature as the set requested in the plugin config. So this is only for cases where the plugin set differs from what is currently being loaded.

We could put tap and @tapjs/test in {project}/node_modules, but, they have to be able to find @tapjs/core, and @tapjs/core has to be able to find @tapjs/test in order to make the TAP class inherit from it.

That last constraint could be loosened by moving the TAP class out of @tapjs/core. By breaking that cycle, which might be a good idea anyway, since nothing else in core really needs TAP to be there, we'd be able to just have these in the project dir somewhere:

  • tap
  • @tapjs/test
  • @tapjs/tap (tk)

Need more exploration of this.


Ok, so, here's a plan that might work for this:

  • split the TAP singleton class into @tapjs/tap, to break the cycle and allow putting that in a different folder from core. This is going to be a big annoying refactor throughout the monorepo, but it's fairly straightforward, and has other benefits (there's kind of a nasty footgun I keep running into, where if @tapjs/test gets loaded before @tapjs/core, it fails trying to class extends undefined, because core loads test to create the TAP class; unlikely other users would run into it, since you have to be in the weeds messing with this stuff to hit it, but that's where I've been hanging out, obviously). Do this regardless, it's good.
  • if the plugin config does not match the built plugins, and tap is installed somewhere other than {project}/node_modules, then tap build will copy the tap, @tapjs/tap, and @tapjs/test classes into {project}/node_modules, and build the Test class at {project}/node_modules/@tapjs/test/test-built (core and all the rest can stay where they are). All three of these are tiny wrapper interfaces, so it's not much to copy.

autobuilds

Right now, the tap executable loads whichever @tapjs/run is resolved from the tap resolved in the cwd, in order to ensure that the runner's tap matches the one being loaded in test files. That guard will have to be extended, or else the runner also has to be copied into the workspace's node_modules, and that's huge (depends on basically everything).

So, instead of the runner just comparing config.pluginSignature to t.pluginSignature, it'll have to load the Test signature from the @tapjs/test resolved from the cwd, which means:

  • @tapjs/test needs to export the plugin signature from a standalone module that doesn't need to pull in core and all the plugins. (Easy enough to add to the build, maybe worth doing regardless.)

Challenges with this plan that have to be overcome

This strategy isn't quite the final draft, at least, not without some adjustments.

You definitely don't want to update tap in the root of the project, and then still be using the old version in tests. So something will have to be worked out to keep those things in sync.

But, if npm sees that there's something in ./node_modules/tap, and "the same" thing in ./packages/workspace/node_modules/tap it'll ""helpfully"" deduplicate it out.

Some approaches to this puzzle which don't work:

  • don't put it in node_modules, put it in .tap/node_modules or something. But then, import() won't pull in the properly built version, tsc won't infer the types from the plugins, etc. The node import() issue could be satisfied by exporting a dynamic import from .tap/node_modules/..., but that won't play nice with static type analysis.

  • put a version on the copied packages, like tap@9999.0.0-18.4.6.copy or something, and have a devDependency on that version. But then, if the node_modules gets deleted, npm will try to install that version, and fail. So that's not great.

  • build the per-workspace Test classes all in the same place, under {ws root}/node_modules/@tapjs/test/test-built, and then branch based on the current project cwd. However, again, this would work for runtime behavior, but fail to infer the types properly.

null status-quo strategy

Just don't do this, and instead, document how you can use t.applyPlugin and that the built tap has to be one tool for the entire monorepo with a single set of built-in plugins.

@npdev453
Copy link
Contributor

Related offtop:

Take a look (maybe here are some answers in) to how day.js deal with plugins:
https://github.com/iamkun/dayjs/blob/dev/test/plugin/timezone.test.js#L8

Also each plugin have it own declare module, what extends root library type
(and its working only after concrete module imported, is summary dayjs type have functions only from really imported modules)
https://github.com/iamkun/dayjs/blob/dev/types/plugin/utc.d.ts#L6

@isaacs
Copy link
Member Author

isaacs commented Oct 10, 2023

Take a look (maybe here are some answers in) to how day.js deal with plugins:

Yeah, that's a nonstarter. The design goal here is:

  • plugins just export some stuff, and export their types
  • the loaded plugins are set as a list of strings in tap's config (which can be set by tap plugin add $plugin)
  • the t object in your test automatically has all that stuff

Having to declare types explicitly is annoying and error prone as a plugin author. And as a plugin consumer, having to explicitly load the plugin set in each test is clunky.

@isaacs
Copy link
Member Author

isaacs commented Oct 11, 2023

Yeah, that's a nonstarter. The design goal here is:

Oh, by the way, that is kind of already possible.

You can use t.applyPlugin at the top of the test to apply whatever plugins you want to the Test class itself.

import tap from 'tap'
import { plugin } from 'my-plugin'
const t = tap.applyPlugin(plugin)
// now t.test() as normal, plugin is applied on this and all child tests
// and typescript knows what's available

However, it obviously won't apply the loaders or config, or update testFileExtensions to decide which test files to load, etc. All of that has to be available to the runner, and in sync with the tests. So that's the behavior I'm trying to make sure can be preserved.

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

No branches or pull requests

2 participants