-
-
Notifications
You must be signed in to change notification settings - Fork 269
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
Comments
Related offtop: Take a look (maybe here are some answers in) to how Also each plugin have it own |
Yeah, that's a nonstarter. The design goal here is:
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. |
Oh, by the way, that is kind of already possible. You can use 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 |
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 theTAP
object exported by@tapjs/core
, and an executable runner that calls the@tapjs/run
executable.@tapjs/core
pulls in the builtTest
class from@tapjs/test
, and theTAP
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, whichtap
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:require('tap')
orimport 'tap'
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 theTAP
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 needsTAP
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:
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 toclass 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.{project}/node_modules
, thentap build
will copy thetap
,@tapjs/tap
, and@tapjs/test
classes into{project}/node_modules
, and build theTest
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'stap
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'snode_modules
, and that's huge (depends on basically everything).So, instead of the runner just comparing
config.pluginSignature
tot.pluginSignature
, it'll have to load theTest
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 nodeimport()
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 thenode_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.The text was updated successfully, but these errors were encountered: