tjones
2020-10-09 12:18
Ok friends! Although I've been maintaining pact-js + node for a while (alongside an excellent team of others), I don't yet have much context for the v3 branch. So I'll be learning along with you
audun.halland
2020-10-09 12:29
_mainly_ here out of curiosity, as I tried to experiment with the v3 branch a while ago. I was a bit surprised that it used a different js API, instead of just a `spec: "v3"` (or similar) parameter.
audun.halland
2020-10-09 12:38
I remember I had problems with 1. managing several mock servers running in parallel in one (jest) test 2. (possibly) several interactions for the same server. My memory may be failing me :)
tjones
2020-10-10 04:24
It looks like the model is one server per interaction- did this cause unexpected problems?
tjones
2020-10-10 04:26
Yeah, I'd like to bring the APIs closer together. The setup and teardown is different out of necessity, because the underlying implementation is different. We should be able to bring the DSL closer together, though.
audun.halland
2020-10-10 12:54
Just think that this api design necessarily causes time consuming refactors, since a lot of our tests have assumed that this will always be possible.
audun.halland
2020-10-10 13:06
Also, as a consequence of javascript working the way it does with JSON deserialisation - it deserializes any valid JSON - we have opted for our JS consumer tests to be generally ?beefier? (a bit more like integration tests) than tests would naturally be in other languages with strict/typed deserialization. My view is that there?s no point having a consumer test if you don?t actually verify that a given response works as intended - you need some code module to send that response into to at least verify that _something works_.
The consequence of that was that it was harder to strictly separate tests to a single endpoint call each, because we have used a higher abstraction layer for our (javascript) tests.
audun.halland
2020-10-10 13:10
And a lot of code bases will be configured given a ?base url? to a specific service (that?s the mock server?s URL under a pact test) and would expect the different provider endpoints to share the same base URL, which will not be possible if there?s one interaction per mock server
audun.halland
2020-10-10 22:27
I see, yes, though still curious as to what exactly makes it need do that out of necessity. I?m a `pact-reference` contributor, I?d love to contribute here also, but I feel I need to understand the architectural choices first
tjones
2020-10-12 07:31
I've fixed the issue you mention about test boundaries by using fixtures - as in:
```expect(apiLayer.someCall()).toReturn(expectedFixture);```
and then in the test where your API layer would be invoked, do:
```const mockApi = { someCall: jest.fn().mockReturn(someFixture)
}```
(or whatever)
tjones
2020-10-12 07:32
I'd been meaning to write that up as a blog post, I should get around to taht.
tjones
2020-10-12 07:33
> And a lot of code bases will be configured given a ?base url? to a specific service
My experience is that it's generally easy to change this on the fly in JS codebases (all the current examples do). But you do raise a good point - ideally Pact wouldn't have an opinion about it
tjones
2020-10-12 07:34
Ah! So, I don't know anything about pact-reference yet - I thought it was some reason to do with the way pact-reference works.
matt.fellows
2020-10-12 09:07
My ideal scenario is that we intercept/proxy the network runtime (think mock) to avoid the need of configuring this sort of thing. It's doable, and I think would solve a big pain point for many pact users (especially newbies)
audun.halland
2020-10-12 21:15
You mean using something like `nock`, that hooks quite deep into node.js network runtime?
tjones
2020-10-15 00:29
@uglyog: Is there a reason the examples spin up one server instance per interaction?
uglyog
2020-10-15 00:38
The previous implementation had to have a long running stateful mock server, because it was a separate process. This is no longer a requirement, so now each test can get a new mock server initialised with the context required for that test. There is now no need to manage different state between each test.
uglyog
2020-10-15 00:41
It also eliminates the HTTP setup/cleanup interactions between the test harness setup/teardown and the mock server. Test setup just configures the pact file in memory, and then that memory is given to the new mock server, and the results are then fetched directly from it.
matt.fellows
2020-10-15 02:27
From memory, point (2) from Audun is still possible with the current design, if not obvious coming from the previous approach
uglyog
2020-10-15 02:38
You can now also have tests run against multiple providers, where each can be a separate mock server.
tjones
2020-10-16 03:45
The reason is because a single mock server means a single URL can be used for all tests.
uglyog
2020-10-16 04:03
There is no need for a single URL for all tests if each test gets the URL it needs
tjones
2020-10-19 01:13
The point is that some application architectures don't work that way. I've personally not architected code that way, but I don't want pact-js to enforce a particular architecture if it doesn't need to
tjones
2020-12-17 07:04
@matt.fellows: We've spoken before about improving the quality of the GraphQL interaction type - since at the moment, it is a regex which sometimes results in incorrect matching. I can't see an issue for this, would you be able to make one, since you're more familiar with the issues?
matt.fellows
2020-12-17 07:06
This is the only issue I?m properly aware of: https://github.com/pact-foundation/pact-js/issues/292
matt.fellows
2020-12-17 07:07
The other is that it doesn?t support mutations as a keyword, and doesn?t support subscriptions (that requires more thinking, it might perhaps fit within the pact message framework)
matt.fellows
2020-12-17 07:08
I?m not sure what I?d raise beyond it, other than I know it?s not perfect. Because GraphQL has its own grammar, it really needs first class support in the ruby/rust engine.
uglyog
2021-01-05 04:26
How do I release a new version of the Pact-JS V3 when the travis build is dead?
tjones
2021-01-20 09:01
@uglyog When you get a moment, could you merge master into V3? I'm without a development machine for a few days
artur
2021-01-20 09:13
@tjones I don't understand what you mean with "promisify on our side with a callback" in https://pact-foundation.slack.com/archives/C9VBGLUM9/p1611133612062000?thread_ts=1611046137.058100&cid=C9VBGLUM9
tjones
2021-01-20 09:16
```new Promise((resolve, reject) => {
const result = someSyncFunction();
if( result.isSuccess() ) {
// or however you check
resolve(result.data); // or whatever
} else {
reject(result.error); // or whatever
}
});```
tjones
2021-01-20 09:19
^ Something like that will let us present sync functions as if they're async. Coupled with a `Promise.resolve(stateFunc())` or similar, I reckon we could then accept both sync and async state functions.
tjones
2021-01-20 09:19
(secondary disclaimer: that code might not be right, I just typed it out into slack)
artur
2021-01-20 10:08
synchronous functions are fine, the problems are async functions. In my case node-fetch returns a Promise and the code does not wait for it to be resolved.
I fail to understand how your solution would help with that, but I also admit I fail to understand Promises in general :wink:
artur
2021-01-20 10:13
I guess it must be here https://github.com/pact-foundation/pact-js/blob/feat/v3.0.0/src/v3/verifier.ts#L73
tjones
2021-01-20 10:15
I see why @uglyog was saying we need to wait for promise support in the native bindings.
tjones
2021-01-20 10:15
I had assumed that we had some sync function called in the native binding, which we could then promisify
artur
2021-01-20 10:17
here is what signal does: https://github.com/signalapp/libsignal-client/pull/65
I think the challenge is simmilar
tjones
2021-01-20 10:18
One of the signal devs says:
> This code is terrifying and I expect will be really fun to debug if/when something goes wrong.
tjones
2021-01-20 10:21
it looks like the `native` library we're using to contact the rust reference implementation also implements a lot of functionality. I think (without much context, so perhaps Ron already considered this and rejected it for some good reason I haven't thought of), we should have the `native` wrapper be as thin as possible and implement the behaviour it needs in JS.
tjones
2021-01-20 10:44
So, I think if Rust doesn't support JS promises, it'll have to pass a `done` callback in to the state function. I don't know enough Rust to be able to modify the wrapper though
artur
2021-01-20 10:57
I think it would be a useful feature to be able to use async code in the states. If its not possible it should be documented (happy to make that docs PR)
tjones
2021-01-20 11:11
if you have a moment to make a feature request issue that would be great, otherwise I'll do it tomorrow
uglyog
2021-01-20 22:11
> we should have the native wrapper be as thin as possible and implement the behaviour it needs in JS.
The problem then is we have to replicate that in all the various languages. By making the language specific part as thin as possible means better support across JS, C++, Python, Go, etc.
uglyog
2021-01-20 22:14
I did put a comment about async provider state callbacks in the readme, but that is on the V3 branch
uglyog
2021-01-20 23:07
I'm thinking that a `done` callback might actually work. I'll have to try that and see how it goes.
tjones
2021-01-21 00:27
@uglyog Right, yes- we don't want to replicate the core pact functionality across all the frameworks. However, the `native` wrapper part is (as I understand it) specific to JS. Is there functionality in there that should be moved to the reference rust implementation?
uglyog
2021-01-21 00:31
The wrapper just translates from JS to Rust and back, it doesn't do much more.
audun.halland
2021-01-25 13:56
I tried out pact-js/v3 again, and it crashes during error reporting. Problem is here: https://github.com/pact-foundation/pact-js/blob/feat/v3.0.0/src/v3/pact.ts#L86, it tries to do `.substr` on something that is not a string.
matt.fellows
2021-01-25 22:11
Gah, that?s embarrassing. I?m off today Audun (public holiday here in Aus) but will set a reminder to look at this tomorrow (even if just raising a tracking issue)
brendan.donegan
2021-01-26 14:57
Hi - I?m looking at using Pact v3 and one of the reasons for this is because we want to use the ?fromProviderState? functionality to inject unique id?s of newly created objects into our requests. I was following this example - https://github.com/pact-foundation/pact-js/blob/feat/v3.0.0/examples/v3/provider-state-injected/consumer/transaction-service.test.js
brendan.donegan
2021-01-26 14:57
This little bit of syntax seems to be problematic for linters: `fromProviderState("\${accountNumber}", "100")`
brendan.donegan
2021-01-26 14:58
apparently it violates eslint rule `no-template-curly-in-string`, and also it seems like prettier wants to remove the \ before the $ sign
brendan.donegan
2021-01-26 15:16
Also I want to use the fromProviderState in the path: but it seems to be failing because it?s returning an object - is it possible to use it in the path?
uglyog
2021-01-26 22:10
This seems like a stupid lint rule, because templates can't be used in normal strings. You can add a suppression for it, or update the lint rules
uglyog
2021-01-26 22:12
How can you automatically convert an Object to a path, which is a string? That will just add [`Object....]` to the path unless it implements `toString()`.
tjones
2021-01-27 00:14
Hmm. I wouldn't expect prettier to want to remove the "\" - the whole pact-js repository is formatted with prettier, so if this were a problem the examples would break. Do you have an unusual prettier config?
Perhaps we should provide an eslint plugin that sorts this out for pact tests- in the meantime, you'll probably have to just turn off the rule on the line. Sorry.
@uglyog - this kind of lint rule is pretty idomatic in js/ts land- it's to discourage code that might be a mistake or has a subtlety you might not notice (whether or not we agree with this value is kind of a side point - the idioms and standards of the community are out there). Do we have much control over the way the templates for provider states are described? The `"\${varName}"` syntax looks a bit jarring in JS, and if it's interfering with the tooling, it would be great to change it to something like `"{{varname}}"` or `"[[varname]]"` or something that doesn't interfere.
uglyog
2021-01-27 00:25
It will have to be translated, because the $ form is what needs to be persisted in the Pact file
artur
2021-01-27 03:56
would it be possible to create a new release of v3? I would like to use https://github.com/pact-foundation/pact-js/commit/6d44059db1917c17e40f128a4b3eedfda16fd2c1
uglyog
2021-01-27 03:58
Current branch is in the middle of a change that not is not currently completed (build is broken too)
uglyog
2021-01-27 04:00
Yeah, and I think the current release process releases the HEAD commit. If we could release from your commit, that would be ideal
tjones
2021-01-27 04:12
This is a good question. We keep master releasable, do we want to do the same with the V3 branch?
uglyog
2021-01-27 04:17
I'll create short lived branches off the V3 one. I've already reverted the change from yesterday
tjones
2021-01-27 05:48
Also, do you think it's worth extracting the wrapped pact core to another package?
tjones
2021-01-27 06:17
While we're on it, the scripts are still publishing the release as a full release, not a beta
uglyog
2021-01-27 06:20
I'll need to investigate if it can be separate. Neon's default is to be alongside the JS code
matt.fellows
2021-01-27 06:31
@tjones looks like you?re messing with pact builds. I?ve just pushed a minor change into pact js to fix an issue with types somebody reported to me. It?s not a widespread issue, but easy enough to sort out. Will you be releasing pact js (master) today?
tjones
2021-01-27 06:34
I'm trying to work out how to get bash to work for releases from windows :disappointed:
matt.fellows
2021-01-27 06:36
(I used to use Commander, but I thought the new WSL had a better shell in it)
matt.fellows
2021-01-27 06:41
the trigger release script failed for me, I think the `sed` matching doesn?t work
matt.fellows
2021-01-27 06:47
if you could please release latest master (i pushed a small change) that?d be ace
matt.fellows
2021-01-27 06:48
```? pact-js git:(master) ? ./scripts/trigger-release.sh
Note: Unnecessary use of -X or --request, POST is already inferred.
* Trying 52.63.231.178...
* TCP_NODELAY set
* Connected to http://api.github.com (52.63.231.178) port 443 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
* CAfile: /etc/ssl/cert.pem
CApath: none
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-RSA-AES128-GCM-SHA256
* ALPN, server accepted to use h2
* Server certificate:
* subject: C=US; ST=California; L=San Francisco; O=GitHub, Inc.; CN=*.http://github.com
* start date: Jun 22 00:00:00 2020 GMT
* expire date: Aug 17 12:00:00 2022 GMT
* subjectAltName: host "http://api.github.com" matched cert's "*.http://github.com"
* issuer: C=US; O=DigiCert Inc; OU=http://www.digicert.com; CN=DigiCert SHA2 High Assurance Server CA
* SSL certificate verify ok.
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Using Stream ID: 1 (easy handle 0x7fa413809000)
> POST /repos/github.com/pact-foundation/pact-js/dispatches HTTP/2
> Host: http://api.github.com
> User-Agent: curl/7.64.1
> Accept: application/vnd.github.everest-preview+json
> Authorization: Bearer *********redacted********************
> Content-Length: 35
> Content-Type: application/x-www-form-urlencoded
>
* We are completely uploaded and fine
* Connection state changed (MAX_CONCURRENT_STREAMS == 100)!
< HTTP/2 404
< date: Wed, 27 Jan 2021 06:46:18 GMT
< content-type: application/json; charset=utf-8
< content-length: 84
< server: http://GitHub.com
< status: 404 Not Found
< x-oauth-scopes: repo, workflow
< x-accepted-oauth-scopes: repo
< x-github-media-type: github.v3; param=everest-preview; format=json
< x-ratelimit-limit: 5000
< x-ratelimit-remaining: 4991
< x-ratelimit-reset: 1611732543
< x-ratelimit-used: 9
< access-control-expose-headers: ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, Deprecation, Sunset
< access-control-allow-origin: *
< strict-transport-security: max-age=31536000; includeSubdomains; preload
< x-frame-options: deny
< x-content-type-options: nosniff
< x-xss-protection: 1; mode=block
< referrer-policy: origin-when-cross-origin, strict-origin-when-cross-origin
< content-security-policy: default-src 'none'
< vary: Accept-Encoding, Accept, X-Requested-With
< x-github-request-id: 1CC2:630A:316CD9:34C060:60110C3A
<
{
"message": "Not Found",
"documentation_url": "https://docs.github.com/rest"
}```
matt.fellows
2021-01-27 07:10
FYI it failed, but just on a call so can?t debug yet: https://github.com/pact-foundation/pact-js/runs/1774885243?check_suite_focus=true
tjones
2021-01-27 07:33
I can't tell either. Might be related to the same issue I had with pact-node, which is that github actions have an insecure world writeable directory, and Ruby complains
tjones
2021-01-27 07:33
There's a script in pact-node that sorts it out, but really we should make sure that we swallow and warn about this error, but don't fail execution
brendan.donegan
2021-01-27 09:05
one question, why wouldn?t you just make the first argument a string which is going to be the name and have the templating be an implementation detail?
brendan.donegan
2021-01-27 09:07
Let me rephrase my question. I would like to use a value taken from the provider state in the path:, is this possible at the moment?
artur
2021-01-27 09:20
CI currently fails https://github.com/pact-foundation/pact-js/pull/591/checks?check_run_id=1775520262
any idea why?
matt.fellows
2021-01-27 11:18
yeah. I don?t believe it?s a fatal issue as far as the binary is concerned
matt.fellows
2021-01-27 11:39
I?m wondering if support for query strings has changed recently in the underlying Ruby binaries
matt.fellows
2021-01-27 11:39
```E, [2021-01-27T22:38:04.829205 #15023] ERROR -- : No matching interaction found for GET /cats?catId[]=2&catId[]=3
E, [2021-01-27T22:38:04.829228 #15023] ERROR -- : Interaction diffs for that route:
E, [2021-01-27T22:38:04.829675 #15023] ERROR -- : Diff with interaction: "a request for cats with given catId" given "i have a list of cats"
Diff
--------------------------------------
Key: - is expected
+ is actual
Matching keys and values are not shown
{
"query": {
- "catId[]": [
- "1",
+ "catId": [
+ "2",
+ "3"
]
}
}
Description of differences
--------------------------------------
* Could not find key "catId[]" (keys present are: catId) at $.query
* Did not expect the key "catId" to exist at $.query
W, [2021-01-27T22:38:04.836258 #15023] WARN -- : Verifying - actual interactions do not match expected interactions.
Incorrect requests:
GET /cats?catId[]=2&catId[]=3 (request query did not match)
W, [2021-01-27T22:38:04.836304 #15023] WARN -- : Incorrect requests:
GET /cats?catId[]=2&catId[]=3 (request query did not match)```
matt.fellows
2021-01-27 11:41
You can see the incoming request has the key `catId[]..` but then it is normalised in the diff
uglyog
2021-01-27 21:47
A string will work, but for things like paths, you need something more like "/users/${id}/orders/${orderId}"
uglyog
2021-01-27 22:02
The provider state callback should be returning a key/value map. You use the keys to lookup the values. You can use them in the path using an expression like "/users/${id}/orders/${orderId}". For that example, the map returned should have an `id` and `orderId` entry in the map.
artur
2021-01-28 05:18
@tjones do you want me to change the example in this PR? https://github.com/pact-foundation/pact-js/pull/590
tjones
2021-01-28 07:01
Ah, sorry, I wasn't clear. Yes please, if it's alright. The rationale is that we get a lot of questions from confused users, so I want to keep the examples as close to real as possible
tjones
2021-01-28 07:01
You'd be surprised what parts of "your real implementation here" end up in code that comes back with questions
tjones
2021-01-28 07:06
So, github actions is currently broken (right after we migrated to it, lol), but we're close to solving the windows build issues.
tjones
2021-01-28 07:06
There are two problems:
1) GitHub Actions is down
2) The latest pact binary has some problems with query strings. Matt and Beth were looking at it today, I'm not sure where they got to
tjones
2021-01-28 07:07
But the latest binary solves the problem with certain ways to invoke pact message on windows, which means we should (finally!) have a green board
artur
2021-01-28 07:20
yes I guess I followed not-so-good design from owncloud, they are using `format=...` everywhre https://doc.owncloud.com/server/10.6/developer_manual/core/apis/ocs/notifications/ocs-endpoint-v1.html#request-parameters
tjones
2021-01-28 07:23
I'd merge now, except it's not trivial to release while gh actions is down :joy:
artur
2021-01-28 07:27
no, don't merge it. I'm not a fan of merging stuff with red CI, that does develop bad habits and then when you finally try to fix CI you find multiple issues stacked above each other
artur
2021-01-28 07:28
actually GH actions seem to run, just a bit slow maybe https://github.com/pact-foundation/pact-js/pull/590/checks?check_run_id=1782129522
artur
2021-01-28 07:30
but the tests fail with the same issue like yesterday https://github.com/pact-foundation/pact-js/pull/590/checks?check_run_id=1782129502
brendan.donegan
2021-01-28 09:19
I see what you mean. That?s ok - it might still be helpful to change the template to ?{{value}}? to avoid fighting with linters
tjones
2021-01-28 09:25
Then users can say something like
```path: `/users/${fromProviderState("id")}/orders/${fromProviderState("orderId")}````
brendan.donegan
2021-01-28 09:28
I had to this instead
``` path: fromProviderState(
// eslint-disable-next-line prefer-template
CLASS_ENDPOINT + '/${sectionRefId}',
SECTION_REF_ID_1,
),```
tjones
2021-01-29 05:18
Yes! Our fancy new green board is thanks to the concerted efforts of you @artur, @milanese.david, @bethskurrie and @matt.fellows . :taco: s for all!
matt.fellows
2021-01-29 09:04
I claim nothing aside from raising an issue saying "let's move off Travis". Thanks folks, so grateful for your help!
artur
2021-02-02 03:29
something went wrong with the beta29 build, only 2 assets (source code uploaded)
matt.fellows
2021-02-02 03:41
Probably me. I tried releasing it the other day using the script. I think it needed manual version updating so I probably am to blame
uglyog
2021-02-02 03:42
No, it is something about how the release is created. It's not triggering the right event
uglyog
2021-02-02 05:52
Oh, that might have been me when I edited the release to get it to kick off. Sorry.
brendan.donegan
2021-02-03 11:00
I?m curious - is there a timeframe for when Pact v3 is going to be officially released? I?m struggling a bit to come up with a way to have Pact v2 and v3 Co-exist in our repo in a way that isn?t confusing to our engineers, so would prefer to switch over wholesale at some point
matt.fellows
2021-02-04 01:26
First quarter this year is the goal. But I see no reason to _not_ have it merged into master rather soon, and released alongside the current (let?s call it v2) package (this way you don?t need to install 2 deps). Would that help your situation?
tjones
2021-02-04 05:33
@matt.fellows: I'd like to overhaul the interface before we do that. We can get them closer to each other
tjones
2021-02-04 05:34
Also, I'm still unclear on what is remaining to do before it is switched over (other than cleanup the types). Is there a list?
tjones
2021-02-04 05:42
@matt.fellows: Been thinking about the windows long path issue. What if we mitigated it slightly by extracting to eg `lib/` instead of `standalone/`? Also, we could name the win32 directory something like `w/`, maybe
omer.moradd
2021-02-04 06:31
Hi
Tim and I having a conversation about how to manage the JavaScript packages and I would like to suggest the following idea - to concentrate all the JavaScript related packages in one place which is basically a Monorepo; And to be more specific then Monorepo with the use of Lerna.
Why Monorepo?
1. Single source of truth
2. No need to maintain so many different repositories.
3. Version management becomes super easy (using Lerna+Changesets / Lerna+Changelog) - can also add conventional commits
4. Easier local development - you can create links between all the packages and thus make sure that each package that depends on another does not break anything
5. Common and uniform processes like linting for example
6. No need to maintain a pipeline for each package separately, it is possible to have one main pipeline for all packages and Lerna will already take care of making the work easy in everything related to testing, building and publication only of what has changed what the last commit
I would love to hear what you think :slightly_smiling_face:
tjones
2021-02-04 06:35
My thoughts are that I'm not sure it would solve any current problems, and would introduce new complexity into the build process
tjones
2021-02-04 06:36
@matt.fellows: I figured out why neither of our branches running npm link on appveyor were merged - npm link doesn't seem to work on appveyor. We're doing it correctly for windows, it works locally
tjones
2021-02-04 06:36
My thinking is that maybe the appveyor builds aren't run as administrator, which is required for npm link to work?
omer.moradd
2021-02-04 06:38
Actually Lerna is taking care of everything :slightly_smiling_face: including the build process and the publishing
omer.moradd
2021-02-04 06:39
So I can guarantee it won?t add a new complexity
I think we should make a POC and see how it works :)
tjones
2021-02-04 06:40
(sure, there's a different command, but for node developers who don't know lerna, there's new things. At the moment, there isn't)
tjones
2021-02-04 06:41
Sure, a POC would be nice, but at the moment I don't see what problem it solves
artur
2021-02-04 06:45
are `Before/After hooks` already implemented in V3? the box is ticked in https://github.com/pact-foundation/pact-js/issues/413 but I cannot find any code in https://github.com/pact-foundation/pact-js/blob/feat/v3.0.0/src/v3/verifier.ts
tjones
2021-02-04 06:47
Oh! I know this one. They're implemented with the boolean on the setup function (true when it is setup, I think?). I'd like to swap this for:
```type stateFunc =
| (params) => Promise<unknown>
| {
setup: (params) => Promise<unknown>;
teardown: (params) => Promise<unknown>
}```
tjones
2021-02-04 06:51
I agree with the advantages you've listed, but to me a monorepo is just one way of implementing those advantages. I don't think moving to a monorepo would solve or improve any problems we currently have:
1) We don't currently have multiple sources of truth. We do have a discoverability challenge whereby you might not know about some js repos, but I don't think that's a problem (and it's easily solved with documentation)
2) I don't think maintaining multiple repos is a problem (or, if it is, solving it seems not worth increasing the learning curve for contributors)
3) Version management is already well solved for the pact projects, We have automated releases and changelogs, and I'm not aware of any pain points.
4) Usually we don't have a problem with changes across multiple projects. The biggest challenge is between pact-node and pact-js, and `npm link` solves this for local development (this also runs automatically in CI for the example repos, so they act as an integration test, kinda)
5) Yes, making lint and formatting uniform would be nice. I don't think it's really costing us, though (and a shared lint package would solve this without the need for a monorepo).
6) Again, I'm not sure this is really costing us. If we want a common pipeline, we could build it without needing a monorepo (I've been thinking of following the pattern used for the ruby gem releases)
tjones
2021-02-04 06:52
I appreciate the enthusiasm to give it a go, and if you feel strongly, feel free to spike it out - but I reckon that there are bigger fish (like fixing up the v3 interface or improving the way we do config) that would be more valuable
artur
2021-02-04 06:53
if the statehandler does a change on the server I revert it again in the state handler. But what if the interaction itself does a change on the server that should be reverted afterwards?
I could use also a stateHandler for that, but looks strange and ugly
`.given('the directory is deleted after the interaction')`
or
`.given('afterHook')`
tjones
2021-02-04 06:54
There's another thing worth considering, which is moving to a monorepo would mean we obscure the commit history. This is the same reason I haven't standardised the formatting between pact-node and pact-js.
omer.moradd
2021-02-04 06:56
All right, I?m probably not yet familiar with all the processes that are currently going on in development
If you feel it adds unnecessary work then it really is not urgent
What?s more, the history of the commits is really very valuable
uglyog
2021-02-04 07:02
Just use the normal test before/after hooks. There is also a callback when the verification process is complete.
tjones
2021-02-04 07:02
One prerequisite before moving to a monorepo is that you can't currently check out `pact-node` and run `npm install` - because it wants to run the download script, which won't yet be compiled
artur
2021-02-04 07:05
the provider test is only one jest test with multiple interactions. So jest will run the afterEach only once I basically need a `afterEachInteraction`
artur
2021-02-04 07:09
OK here the idea: its only about naming, `given()` works just fine, if there would be an alias called `after()` the naming would be nicer
tjones
2021-02-04 07:10
@uglyog this feels like another argument for letting pact-js tell rust which interaction is ready to verify
tjones
2021-02-04 07:12
@artur Right, but you're tearing down that particular state, not running an afterEach
artur
2021-02-04 07:21
thinking about it, actually an afterEach is not what I need. I need an action to revert the effect of the interaction itself. I should be able to do that with the `given` even if I have multiple of them. A given can revert its own effect that it caused during setup or it cannot do anything in the setup phase and revert the effect of the interaction in the teardown phase (I would like to be able to call that `provider.after()` or simmilar
The only problem I can see is that I don't know the order of givens so if the reverting has to happen in a specific order I might be in trouble
artur
2021-02-04 08:41
I've created a feature request https://github.com/pact-foundation/pact-js/issues/597
Would love to hear your input
brendan.donegan
2021-02-04 09:19
I?d like to feel confident it will be stable and their won?t be too many interface changes though - I unfortunately don?t have a lot of time to overhaul our Pact tests as changes are made, especially now we have a lot of them
matt.fellows
2021-02-07 23:05
Sorry, a little late to this discussion. I?ve also suggesting looking into Lerna, but admittedly I have never used it so a shift would need to produce significant gains (and incremental gains for what looks to be a big effort, might better be used on other things - e.g. the v3 migration, API cleanup and various features people want!).
I?m not sure how big a learning curve Lerna is, if that?s the main counter argument though.
matt.fellows
2021-02-07 23:06
1. We already produce 3 packages from that repository (the new v3 beta release, pact-web and the existing package). It?s a bit clunky the way that?s currently setup, and having v3 in a long running branch is a risk for all of the obvious reasons
matt.fellows
2021-02-07 23:07
2. whilst having multiple repos isn?t a huge burden, having to wait for upstream projects to build, release etc. is a small time overhead and it would be much nicer for pact-node and pact-js to co-locate
tjones
2021-02-08 07:30
I'm not sure having them in the same place would speed up build + releases - the problem would still exist
tjones
2021-02-08 07:31
I think the production of three packages from the repo is probably better treated by splitting out pact-web, and bringing the beta release into the main branch
matt.fellows
2021-02-08 08:48
I would share code and have pact JS not depend on pact node anymore, but still publish pact node for those who still want to use it without Pact JS (for whatever reasons)
matt.fellows
2021-02-08 08:49
Agree in general, and the point about where to expend effort is one of the bigger considerations
matt.fellows
2021-02-08 13:00
just back into this thread - so, how did you re-trigger the release Ron?
matt.fellows
2021-02-09 00:46
I thought be checking the ?pre-release? box and then re-doing that, it would trigcger it. Apparently that?s not an ?edit?
matt.fellows
2021-02-09 00:46
i.e. according to GH actions, these are the only 2 times that workflow has run - that lead me down a separate rabbit hole to figure out how you ran those bits
artur
2021-02-09 03:42
there is a `master` release https://github.com/pact-foundation/pact-js/releases
is that intended?
matt.fellows
2021-02-09 03:50
Thanks for reminding me. It seems that can be a funny side effect of a failed release
audun.halland
2021-02-09 10:43
Hey, testing pact-js v3 again :slightly_smiling_face: latest beta. I have a test that completely stalls, no idea what?s going on (I also sometimes see a segfault). Is there some way to see native/Rust term debug while running a test through Jest?
matt.fellows
2021-02-09 10:56
Just check you haven?t sent a promise or something to the callbacks. I don?t think they support promises yet - maybe they just go into the ether?
audun.halland
2021-02-09 10:59
Well, I use `provider.executeTest(async (mockServer) => ...)`. I?m thinking that I won?t get very far with I/O in javascript without promises?
audun.halland
2021-02-09 11:01
LOG_LEVEL works. The last Rust log line that I see is `[2021-02-09T11:00:22Z DEBUG pact_matching::models] Merging pact with file "pacts/foo-bar.json"`
audun.halland
2021-02-09 11:03
Just saw in the logs that there was a recent change related to locking the file, unsure whether it?s related
audun.halland
2021-02-09 11:17
I can induce the SIGSEGV by forcing a ?Request did not match pact? thing:
```[2021-02-09T11:15:11Z DEBUG pact_matching] --> Mismatches: [BodyMismatch { path: "$.variables.slug", expected: Some(b"\"lol\""), actual: Some(b"\"movedFromHere\""), mismatch: "Expected \'lol\' to be equal to \'movedFromHere\'" }]
[2021-02-09T11:15:11Z DEBUG pact_mock_server::hyper_server] Request did not match: Request did not match - Request ( method: POST, path: /graphql, query: None, headers: Some({"Content-Type": ["application/json"]}), body: Present(1003 bytes, application/json) ) 0) $.variables.slug -> Expected 'lol' to be equal to 'movedFromHere'
error Command failed with signal "SIGSEGV".```
audun.halland
2021-02-09 11:19
Du you want me to instead report issues on GitHub or is it ok to just discuss it here?
matt.fellows
2021-02-09 12:35
I think it?s just the state callbacks/filters that don?t support promises. But yes, JS === promises!!
uglyog
2021-02-10 02:14
It has to be relating to the file lock, as it will block the process until the lock can be aquired.
uglyog
2021-02-10 02:16
@matt.fellows I'm going to update the locking code to not block, but try acquire the lock with an backoff.
matt.fellows
2021-02-10 02:27
That will give me a chance to test the updates to the JS release script :crossed_fingers:
uglyog
2021-02-10 04:01
Can you double check the logic? All the tests are passing, but :man-shrugging:
https://github.com/pact-foundation/pact-reference/commit/f34629e19f7898a5722db2a288113b0b98b7e858
https://github.com/pact-foundation/pact-reference/commit/6f13f495a8cd849971be9d3be0d398a95adf14bc
audun.halland
2021-02-10 04:54
Will you require interprocess locking? If not, I suggest having a dedicated thread that has exclusive access to the pact file. Other threads can communicate with it by sending/receiving commands through channels, this will also avoid thread::sleep when you want to do pact file operations in async contexts
uglyog
2021-02-10 05:01
There is a process lock (actually added by you using a Mutex), but the issue I think is with Jest it runs multiple processes.
audun.halland
2021-02-10 05:12
Ah, ok. I recall. I also recall something about jest running in processes, yes. I take it back
audun.halland
2021-02-10 11:56
but, `thread::sleep` is ?blocking? in the definition of blocking that I use
matt.fellows
2021-02-11 03:36
ah so it has. Just deleted it. I?ve had to make a few adjustments because of the previous setup. Each release we?re getting closer (the last issue is that it dispatched the native build, but incorrect tag was applied)
matt.fellows
2021-02-11 03:36
`master` is what happens when the native release is triggered without context (I think)
matt.fellows
2021-02-11 03:36
I?ve inlined the v3 release with the native artifacts - so it should work better now
audun.halland
2021-02-16 13:19
@matt.fellows I debugged this, and it?s not directly related to pact v3. It?s this bug (related to the `import()` _function_ in node): https://github.com/nodejs/node/issues/35889
Even if the bug is not directly related, it only manifests itself when running together with pact?s native module.
audun.halland
2021-02-16 23:16
maybe other users will see it too. It appeared in my tests because of an ?async? `import()` call in our codebase deep inside some completely unrelated code
artur
2021-02-22 03:59
I have this PR open https://github.com/pact-foundation/pact-js/pull/603
can it get merged or should I close it? Sorry for pushing, I just would prefer to get it off my list
artur
2021-02-22 05:14
can that be the difference: https://github.com/pact-foundation/pact-js/blob/69a6e5b5263750bbef84ae7cadbc20d95294ee1c/scripts/ci/build-and-test.sh#L22
for the linux tests we make sure the current PR code is used in the tests
in appveyour I cannot find anything similar
tjones
2021-02-22 05:16
(this is super easy to miss - Matt and I both had half-branches adding the `npm link`, too)
artur
2021-02-22 07:38
the problem is that the broker docker cannot be run in windows :disappointed:
so https://github.com/pact-foundation/pact-js/blob/ae8397af7a30af95fdcade718a6bd5c458da9e25/examples/v3/e2e/test/publish.js#L11 fails
matt.fellows
2021-02-22 09:19
good spot. That check looks backwards to me (maybe). I didn?t realise docker is running on that build at all. I mean, I can?t see where it starts on the github build
matt.fellows
2021-02-22 09:21
so this was the commit: https://github.com/pact-foundation/pact-js/commit/4f1b87a4d1771974208ae05f3581db30d831ba01
artur
2021-02-22 10:05
I'm happy to try to fix CI. lets see what the :robot_face: is thinking of https://github.com/pact-foundation/pact-js/pull/613
tjones
2021-02-22 10:06
The other tests expect the public test broker to be available. This isn?t _great_ practice, but it is a problem that:
1) Affects all the others anyway
2) We haven?t hit issues because of
artur
2021-02-22 10:08
anybody who runs the tests locally will hit the brocker anyway. At least if there is a problem we will notice in CI and not only people getting annoyed because examples don't run
uglyog
2021-02-23 22:46
The artefacts are attached to a release with a double v in the tag while the actual release has none
matt.fellows
2021-02-24 00:05
verified by running a local install, and downloading the referenced file
artur
2021-02-24 07:36
@tjones sorry I don't get which vars in https://github.com/pact-foundation/pact-js/pull/613 should be const?
tjones
2021-02-24 10:21
I think it?s worth changing at some point to make the style immutable. But you?re right that my feedback was wrong, my apologies :pray:
tjones
2021-02-24 10:31
So, generally in pact-js we?ve got things in an immutable style. So, where we have:
```let consumerVersion = "1.0."
if (process.env.APPVEYOR) {
consumerVersion = consumerVersion + process.env.APPVEYOR_BUILD_NUMBER
} else if (process.env.GITHUB_ACTIONS) {
consumerVersion = consumerVersion + process.env.GITHUB_ACTION
} else {
consumerVersion = consumerVersion + Math.floor(new Date() / 1000)
}```
we might introduce a function, so we can do something like:
```const getVersion = (versionBase: string) => {
if (process.env.APPVEYOR) {
return `${versionBase}${process.env.APPVEYOR_BUILD_NUMBER}`;
}
if (process.env.GITHUB_ACTIONS) {
return `${versionBase}${process.env.GITHUB_ACTIONS}`;
}
return `${versionBase}${Math.floor(new Date().getMilliseconds() / 1000)}`;
};
const consumerVersion = getVersion('1.0');```
tjones
2021-02-24 10:32
The main advantage of this style is that it?s easier to reason about what values are as you read through the code. It?s still possible for values to change on you if you have objects or arrays, but any values that are primitives will change
tjones
2021-02-24 10:34
Also, as an aside, I?m working on a way of introducing pact-friendly versions so that users don?t have to worry directly about the versions
audun.halland
2021-03-02 22:56
Hi! `atLeastOneLike`, `atLeastLike` from `MatchersV3`, are they supposed to work? When I use them, the mock server appears _return_ the matcher itself, I get `{"min":1,"pact:matcher:type":"type","value":VALUE_I_EXPECT_TO_SEE}`
audun.halland
2021-03-02 23:21
ummm. forget it. I think I found a bug in my own code (porting from pact-js ?v2?)
audun.halland
2021-03-03 00:36
Another question :flushed: : Using `MatchersV3.datetime("yyyy-MM-dd'T'HH:mm:ss.SSSX", SOME_EXAMPLE_INPUT);`, the mock server does not return the example input, but instead generates a new date from the current system time. This breaks my equals-checks that?s part of my pact consumer ?unit?-test
matt.fellows
2021-03-03 00:46
ah, that should be easily fixed (he says without looking at the code). Mind creating a request on GH?
audun.halland
2021-03-03 00:53
I wasn?t able to see how this can work even in `pact-matching`, but I didn?t study it for more than a few minutes
matt.fellows
2021-03-03 00:55
ah fair call. Just get it to where you need it, and we can re-route it to where it belongs
uglyog
2021-03-03 05:23
This needs to be fixed in JS. It is setting a generator. That will always replace the value. It needs to not set the generator if there is an example value.
audun.halland
2021-03-03 08:14
Cool, does it need to be fixed for `date` and `time` as well? https://github.com/pact-foundation/pact-js/blob/c0ca78b6995354154ed387f18c1d896789acf778/src/v3/matchers.spec.ts#L360
matt.fellows
2021-03-03 08:44
Yep, I just fixed that real quick. I was doing other refractors and thought "I'll just get the fix you Audun out rather than boil the ocean"
matt.fellows
2021-03-03 10:12
i?ll be testing the release process again later with the changes :point_up:
tjones
2021-03-09 06:29
^ @matt.fellows: Do you happen to know how to configure dependabot? I thought it used to make PRs with `fix:`, but maybe that was synk.
tjones
2021-03-09 06:32
Looks like we can use the config file at https://dependabot.com/docs/config-file/ , but doing it per repo feels clumsy
matt.fellows
2021-03-09 09:54
I recall looking into org wide configs a while back, and it didn?t seem to be a thing
tjones
2021-03-09 10:44
I couldn?t find it either. My thinking was that it?s a fix because it?s a vulnerability update to a dependency
tjones
2021-03-12 04:58
@matt.fellows (and any other contributors): I?ve introduced clearer types for the matchers in the V3 branch. Following the language that @uglyog was using in the code, I think we have:
1) `Matchers` which are specific matchers for a field or object or array
2) `AnyJson` which is for the concrete types serialisable into JSON (other names could be `AnyJsonValue` or `JsonCompatibleValue`). This is for the raw values in a request/response.
3) `Templates` which are a combination of matchers and raw values (ie, the value you pass to `body`).
Thoughts on those names?
tjones
2021-03-12 05:06
@bethskurrie: I know you?re not a JS maintainer, but I?d welcome your thoughts the above names too.
tjones
2021-03-12 05:21
1) Matcher:
```Matchers.like("someString")```
2) AnyJson:
```body: {
a: "field",
some: ["array", "of", "exact", "strings"]
}```
3) Template:
```body: {
a: Matchers.like("field"),
some: ["array", "of", "exact", "strings"]
}```
tjones
2021-03-12 05:27
@matt.fellows: I propose we publish a version of pact-web that prints a warning on install, linking to the issue about removing it. Might get more traction on the issue that way
artur
2021-03-24 06:19
@uglyog I don't understand what you mean by https://github.com/pact-foundation/pact-js/pull/634#issuecomment-805511357
uglyog
2021-03-24 06:30
The body gets encoded into a JSON structure with the matching rules and generators, and then gets decoded on the other side. By replacing the JSON structure with the actual value for the body, you loose the matching rule.
uglyog
2021-03-24 21:53
No, sorry, the underlying Rust code. I'll have some time this weekend to sort it out
github
2021-04-19 00:57
GitHub app is successfully upgraded in your workspace :tada:
To receive notifications in your private channels, you need to invite the GitHub app `/invite @GitHub`
github2
2021-04-21 09:47
[pact-foundation/pact-js] New release _https://github.com/pact-foundation/pact-js/releases/tag/v9.15.5_ published by github-actions[bot]
matt.fellows
2021-04-22 02:25
> ^ I made a card for the checksums / tar.gz issue. If you have an OSS day this week and don?t have anything in mind (lol), that one would be helpful to me
@tjones is this about combining the various FFI artifacts into a single file with a checksum? That might be hard, because each sub package is currently released separately. The checksum is easy, of course, and was on my backlog
matt.fellows
2021-04-22 02:26
Also, are you happy with the backlog created https://github.com/pact-foundation/pact-js/projects/3? If so, I?ll convert to issues to track
tjones
2021-04-22 02:43
Maybe we should have a v3 release tag or something so we can see the whole status
matt.fellows
2021-04-30 02:36
My thoughts - we should just go back to bundling the binaries into the package
matt.fellows
2021-04-30 05:37
Yeah, but then we actually fixed part of the problem that was making the package really big
matt.fellows
2021-05-09 09:04
Lots of stuff going on @tjones, I'll be on a little later. Ping me if there is anything on the top of your list to look at, otherwise I'll go thru open issues/PRs or look into things we discussed last time
github2
2021-05-21 13:58
[pact-foundation/pact-js-core] New release _https://github.com/pact-foundation/pact-js-core/releases/tag/v11.0.0_ published by github-actions[bot]
github2
2021-05-21 15:53
[pact-foundation/pact-js] New release _https://github.com/pact-foundation/pact-js/releases/tag/10.0.0-beta.35_ published by github-actions[bot]
github2
2021-05-21 15:59
[pact-foundation/pact-js] New release _https://github.com/pact-foundation/pact-js/releases/tag/v10.0.0-beta.35_ published by github-actions[bot]
artur
2021-05-25 05:44
I'm trying to rebase https://github.com/pact-foundation/pact-js/pull/636
fixed the conflicts etc. but CI fails with
```Error: src/v3/pact.ts(177,31): error TS2345: Argument of type 'string | number | boolean | JsonArray | JsonMap | TemplateMap | TemplateArray | Matcher<unknown> | null | undefined' is not assignable to parameter of type 'string | number | boolean | null | undefined'.
Type 'JsonArray' is not assignable to type 'string | number | boolean | null | undefined'.
Type 'AnyJson[]' is not assignable to type 'string'.```
I now found out that when I completely clean the repo `git clean -dfX` and run `npm ci` or `npm install`
`native/index.node.d.ts` and `native/src/lib.rs` are modified
Are those files pulled from somewhere or why are they modified on install?
artur
2021-05-25 06:30
I think those files are grabbed from the release tarballs
older release tarballs don't contain `index.node.d.ts` but since beta.34 they do
compare
https://github.com/pact-foundation/pact-js/releases/download/v10.0.0-beta.33/node-v72-linux-x64-glibc.tar.gz
with
https://github.com/pact-foundation/pact-js/releases/download/v10.0.0-beta.34/node-v72-linux-x64-glibc.tar.gz
artur
2021-05-25 06:51
I found now that the download happens through the postinstall script, but cannot find the change that made newer releases contain that extra file
tjones
2021-05-25 07:12
Hmm. I?ll take a look at this tomorrow. Using neon has caused a number of issues- we?re looking at using the ffi bindings instead, which will obviate this entirely
artur
2021-05-25 08:11
easy fix for CI would be, not to download the release but actually build it
`node-pre-gyp install --fallback-to-build=false` for local and `neon build --release`for CI
artur
2021-05-26 11:52
@matt.fellows after https://github.com/pact-foundation/pact-reference/pull/104, would we even need https://github.com/pact-foundation/pact-js/pull/668 or will those env. variables be passed to rust when set before running tests in JS?
matt.fellows
2021-05-26 12:17
Yes you will. Long term no, but the current work you did doesn't navigate that path
artur
2021-05-26 12:21
OK, tomorrow I will try to finish that PR by adjusting the docs and adding some tests
tjones
2021-05-26 14:03
A bit of a heads up - we?re looking at replacing the rust section of the beta with FFI calls via pact-core - the reason for this is that the current beta version has trouble with promises, and a substantial amount of rust code required to get the bindings in
tjones
2021-05-26 14:04
However, it?s not quite ready to go yet, so your PR is very much appreciated, and we?ll get it in and released as soon as Matt can review it
artur
2021-05-28 03:26
the new tests pass on the linux builds in CI but not on appveyor
https://ci.appveyor.com/project/pact-foundation/pact-js/builds/39348907/job/ps32xjlpefgv1pf8#L3562
could it be that the appveyor build actually does not compile the changed rust code? :thinking_face:
artur
2021-05-28 03:31
hmm it should, at least according to the .ps1 script - just don't find all the output I would expect
github2
2021-05-31 07:04
[pact-foundation/pact-js] New release _https://github.com/pact-foundation/pact-js/releases/tag/v10.0.0-beta.36_ published by github-actions[bot]
artur
2021-06-02 09:51
@tjones its a draft, because I wan to add tests, but can you have a look it you are happy with the approach https://github.com/pact-foundation/pact-js/pull/678
tjones
2021-06-02 11:08
Yes, this looks like an improvement to me. I like that you?re removing the `as` cast - I think in general, we should avoid using `as`.
tjones
2021-06-02 11:09
Also, do you think we should rename `content` to `text` so that the `context`/`content` misread can?t happen again?
tjones
2021-06-02 11:10
I?d like to see the xml support done in a less heavyweight way, but I?m not really sure what that would look like
tjones
2021-06-02 11:11
I don?t really tend to work with XML in JS, so I?m not sure what patterns people expect
artur
2021-06-02 11:12
the XML support was our main reason to switch to V3, otherwise to construct dynamic XMLs with regex was hell
artur
2021-06-02 11:18
nested `appendElement` https://github.com/owncloud/owncloud-sdk/blob/master/tests/capabilitiesTest.js#L32-L40
tjones
2021-06-02 11:19
sorry, I meant, what kind of patterns are used in JS for handling XML (outside of pact)
artur
2021-06-02 12:07
those are DAV requests handled by this lib https://github.com/perry-mitchell/webdav-client
I would have to dig around to find how they construct the XML
github2
2021-06-06 03:26
[pact-foundation/pact-js] New release _https://github.com/pact-foundation/pact-js/releases/tag/v10.0.0-beta.37_ published by github-actions[bot]
github2
2021-06-21 13:10
[pact-foundation/pact-js] New release _https://github.com/pact-foundation/pact-js/releases/tag/v10.0.0-beta.38_ published by github-actions[bot]
github2
2021-06-21 14:30
[pact-foundation/pact-js-core] New release _https://github.com/pact-foundation/pact-js-core/releases/tag/v11.0.1_ published by github-actions[bot]
github2
2021-06-21 14:35
[pact-foundation/pact-js] New release _https://github.com/pact-foundation/pact-js/releases/tag/v9.16.0_ published by github-actions[bot]
github2
2021-06-23 07:43
[pact-foundation/pact-js] New release _https://github.com/pact-foundation/pact-js/releases/tag/v10.0.0-beta.39_ published by github-actions[bot]
github2
2021-06-24 03:54
[pact-foundation/pact-js] New release _https://github.com/pact-foundation/pact-js/releases/tag/v10.0.0-beta.40_ published by github-actions[bot]
github2
2021-06-26 13:25
[pact-foundation/pact-js] New release _https://github.com/pact-foundation/pact-js/releases/tag/v10.0.0-beta.41_ published by github-actions[bot]
github2
2021-06-29 03:03
[pact-foundation/pact-js] New release _https://github.com/pact-foundation/pact-js/releases/tag/v10.0.0-beta.42_ published by github-actions[bot]
github2
2021-07-18 12:20
[pact-foundation/pact-js] New release _https://github.com/pact-foundation/pact-js/releases/tag/v10.0.0-beta.43_ published by github-actions[bot]
github2
2021-07-22 06:35
[pact-foundation/pact-js] New release _https://github.com/pact-foundation/pact-js/releases/tag/v10.0.0-beta.44_ published by github-actions[bot]
github2
2021-08-02 00:44
[pact-foundation/pact-js-core] New release _https://github.com/pact-foundation/pact-js-core/releases/tag/v11.1.0_ published by github-actions[bot]
github2
2021-08-11 06:10
[pact-foundation/pact-js-core] New release _https://github.com/pact-foundation/pact-js-core/releases/tag/v11.1.1_ published by github-actions[bot]
tjones
2021-08-23 11:35
Friends, I've created #libpact_ffi-users - since there's quite a bit of discussion about the rust pact ffi bindings in the different language channels, but the discussions cover quite a bit of the same ground. Please join if this is useful or of interest to you!
matt.fellows
2021-08-24 04:44
hmm, so this is annoying: https://github.com/pact-foundation/pact-js/runs/3407020912?check_suite_focus=true#step:4:120
matt.fellows
2021-08-24 04:44
There must be a way to detect this crap at compile time. It feels like whack a mole
matt.fellows
2021-08-24 05:03
do you know if there is a way to detect that at pact-js-core packaging time?
tjones
2021-08-24 05:05
I don't think so. But, ideally we shouldn't be exposing types from someone else.
tjones
2021-08-24 05:06
it's also because the typescript settings in pact-js are aggressive. Because I like it like that, and we're a library so we should be conservative in making sure that we expose types correctly.
matt.fellows
2021-08-24 06:09
totally, but Pact JS core should probably be aware of this potential issue right?
tjones
2021-08-25 01:38
JS maintainers - @matt.fellows / @yousafn / @artur (I think?) - I'd like to standardise the npm targets for the JS repos, since theres `npm run build` in some and `npm run dist` in others (and the same inconsistencies for other targets, like formatting and linting). I was thinking of standardising them to be about the purpose, rather than the how (eg `npm run format:fix` rather than `npm run prettier:write`). Do any of you have any opinions on these?
tjones
2021-08-25 01:39
Also, I think Artur mentioned a while back that it might be better to not rely so heavily on the `pre` scripts (at the moment, `prerelease` in some of the repos runs a lot of the build tasks, which results in doubled build times sometimes). I think he's right, and unless there are objections I'll tidy that up too (and modify the CI scripts accordingly). Thoughts on that are also welcome.
matt.fellows
2021-08-25 01:54
yeah, the build system has gotten a bit confusing, because of the use of scripts which make the process less visible
matt.fellows
2021-08-25 01:54
in that case, mixing the `pre` and `post` with scripts that have behaviour, mean you can?t see ?at a glance? what each thing does
matt.fellows
2021-08-25 01:54
I think the scripts are in a lot better shape, thanks to the great work you?ve done
tjones
2021-08-25 01:55
Yep. The rationale behind heavy use of pre and post was that it would make it harder to run the scripts in the wrong order
tjones
2021-08-25 01:55
but I think the additional load on local build times + extra confusing scripts means that I made the wrong call
matt.fellows
2021-08-25 01:56
I?m sure that if we started with a blank slate things would look different, but evolution isn?t always pretty!
tjones
2021-08-25 01:56
(I tagged @artur above because I think he had some really well reasoned thoughts on this last time we talked about it, but I don't remember them and can't find them in the history)
yousafn
2021-08-25 08:24
Sounds very sensible my friend, anything to reduce cognitive load when working across multiple places is welcomed!
artur
2021-08-25 12:35
I haven't looked into the scripts for a while, but when I did I remember having a hard time to understand how the build works, so I really appreciate anything that reduces complexity.
artur
2021-08-25 12:36
@tjones I like your idea about renaming the scripts to be about the purpose :+1:
tjones
2021-09-01 00:39
> I remember having a hard time to understand how the build works,
Ah, this is good feedback. I'll make sure it gets documented too
matt.fellows
2021-09-14 07:10
Bugger, the latest release didn?t go out correctly:
? https://github.com/pact-foundation/pact-js/runs/3581821861?check_suite_focus=true
? https://github.com/pact-foundation/pact-js/actions/runs/1227643322
matt.fellows
2021-09-14 07:10
Looks like Node 16 didn?t build, and the release bailed, and the second attempt to run wouldn?t work because of empty commit?
matt.fellows
2021-09-14 07:23
@uglyog do you know where the source code is for this docker container? https://hub.docker.com/r/pactfoundation/pact-js-musl ?
tjones
2021-09-14 07:27
All that would be in that release is node 16 support, which was meant to be in the last one
tjones
2021-09-14 07:28
Also, M1 users can't install the beta because we don't build those artefacts for M1
matt.fellows
2021-09-14 07:29
from the error I saw from tugtugtug that doesn?t work either, because the cargo.yaml file isn?t there
tjones
2021-09-14 07:45
yep, I thought I fixed that, but maybe I didn't push it to the right branch or something
matt.fellows
2021-09-14 07:51
The image is on the way up now (it?s big). I better head to do dinner with kids etc., but it should upload in the next 5-10 minutes
matt.fellows
2021-09-15 12:46
OK so I?ve patched the latest two releases to ensure the musl libs are there
matt.fellows
2021-09-15 12:46
any new releases we?ll need to manually do the musl build locally and upload until we remove neon
matt.fellows
2021-09-15 12:47
I?d rather spend my energy porting to the new core than working out why that artifact isn?t building
tjones
2021-09-16 15:23
@matt.fellows - I enabled node 16 builds, but it has npm version 7, which has better dependency resolution and is now complaining about us violating peer dependencies during npm link.
tjones
2021-09-16 15:36
anyway, I'm trying it with --legacy-peer-deps, I reckon that will fix node 16, but I don't know if the earlier builds will fail with unknown arguments or something. I'm going to head to bed, will see if it built in the morning
tjones
2021-09-16 15:36
I reckon we should do the same as the musl build - defer fixing it until after v10 is out.
tjones
2021-09-16 15:37
Maybe we can refactor the examples to not use npm link, but I can't easily see a nice clean way of doing it
tjones
2021-09-16 15:38
Anyway, the peerdep it was complaining about is jest-pact, which (correctly) reports that it isn't compatible with v10. Since we control it, maybe we can come up with a creative option there.
matt.fellows
2021-09-17 09:49
I think your proposal of a pre-release makes a lot of sense, or something like it
matt.fellows
2021-09-17 09:49
I do like that they are a form of smoke tests, or a feature spec (in lieu of that), but not at the expense of making the 99% of the time where they are just examples hard (and in the current moment, seemingly impossible)
tjones
2021-09-17 10:37
I also like that it forces the examples to be working. None of this "oh that example hasn't been updated in 4 versions"
matt.fellows
2021-10-29 06:44
I?d have thought it would still run, but unless a newer incompatible npm was installed along with it?
matt.fellows
2021-10-29 06:45
bugger, can?t re-run the release step because it tries to re-generate notes :cry:
matt.fellows
2021-10-29 06:56
I think we can just re-upload the node libs from the previous version, because there is nothing in the index.node that has changed right?
tjones
2021-10-31 04:40
I don?t know. Once it?s EOL it?s not expected to work, so it might work but also it might not.
tjones
2021-10-31 04:40
When node 13 was supported I remember we had problems with it that were fixed by bumping the version
artur
2021-11-01 08:38
any idea what could cause this crash https://github.com/pact-foundation/pact-js/pull/759#issuecomment-951914617 ?
```#FailureMessage Object: 0x7ffc796f1fc0
1: 0xaa6321 [/opt/hostedtoolcache/node/14.18.1/x64/bin/node]
2: 0x1a3b274 V8_Fatal(char const*, ...) [/opt/hostedtoolcache/node/14.18.1/x64/bin/node]
3: 0xe9afd9 ```
artur
2021-11-01 11:10
after removing all old links I now reproduce that crash locally :thinking_face:
matt.fellows
2021-11-01 13:14
Might be related to the crash in node FFI lib that Tim has been helping with resolving
tjones
2021-11-01 14:30
I've been trying to get them to merge a fix that someone else made, but I don't really understand it, don't know C++ very well (although I know the C standard very well), and the maintainer doesn't appear to have any time to work on it.
tjones
2021-11-01 14:31
I'm very disappointed with the state of bindings to other languages from node. The neon approach is only partially complete, and the ffi approach seems to be unmaintained and confusing too.
tjones
2021-11-01 14:33
I'm not really sure what we do next. I think our options are:
1) Write a pact core natively in JS
2) Write proper C ffi bindings that work (ie, release a replacement for node-ffi)
I... don't like either option because they're both a lot of work. We also have:
3) Hope that neon complete their implementation to a point where we can support promises and don't have to have Rust in the JS codebase
4) Hope that node-ffi becomes actively maintained
tjones
2021-11-01 14:35
neon did have partial support for promises merged recently, so that's potentially hopeful. However, there are so many other maintenance challenges with neon that I'm reluctant to champion it moving forward.
matt.fellows
2021-11-01 23:27
I don?t think Neon is a realistic option for us. Even if it supports promises, there is too great an ongoing overhead to use it (each major Node release for example will be held up on Neon support, and as we saw with Node16 that is sometimes non-trivial to support)
artur
2021-11-02 07:50
I've asked in our team but nobody got exited to take over that C++ PR :disappointed:
matt.fellows
2021-11-15 03:57
@tjones any ideas what?s going on here? I went to pick up https://github.com/pact-foundation/pact-js/issues/750 (adding a flag to the verifier) but getting a compile error in the `pact-node` branch of `pact-js-core`:
```> npm run clean && tsc
> @pact-foundation/pact-node@10.14.0 clean /Users/matthewfellows/development/public/pact-js-core
> rimraf '{src,test,bin,standalone}/**/*.{js,map,d.ts}' 'package.zip' '.tmp' 'tmp', 'standalone/{darwin,linux,win}*'
src/verifier/types.ts:1:26 - error TS2307: Cannot find module '../logger/types' or its corresponding type declarations.
1 import { LogLevel } from '../logger/types';
~~~~~~~~~~~~~~~~~
Found 1 error.```
tjones
2021-11-15 03:59
> src/verifier/types.ts:
This file doesn't exist in that branch, so I don't know what's going on.
matt.fellows
2021-11-15 04:00
actually you?re probably right, maybe the main branch is hanging about. :facepalm:
artur
2021-11-19 05:12
@tjones @matt.fellows
writing a pact core natively in JS would that be a technically very hard thing?
I'm thinking if we can put some of our resources into it
tjones
2021-11-19 05:13
The problem is that it goes against the plan of having all the client libs using the Rust core
bethskurrie
2021-11-19 05:22
Ron has already written a pact implementation at least 4 times. It's not a small thing, and keeping all the implementations in sync with each other and performing the same matching logic and having the same behaviour, across 10 different languages...
bethskurrie
2021-11-19 05:22
And everyone is keen to start an implementation, but are they willing to maintain it 5 years down the track when they're moved on to another project, and maybe aren't even developing any more?
bethskurrie
2021-11-19 05:23
We have seen it multiple times - someone gets enthusiastic, starts an implementation, then can't keep going, and the core team gets left supporting all the users who want to know why it's not working or doesn't have the latest features.
artur
2021-11-19 05:29
Yes I can see the issue @bethskurrie
And honestly I'm not enthusiastic about a new implementation at all and not sure if we even have the resources to start. But for the current project (and most likely for a project at the door) I need the V3 branch to work. So wanted to explore what options there are
matt.fellows
2021-11-19 05:58
It?s a big piece of work, especially with the new plugin architecture coming too.
matt.fellows
2021-11-19 05:59
We need a solution to the problem though, because as you have duly noted, we are currently blocked
matt.fellows
2021-12-02 22:14
Lord, that deprecated package - a package > 4 years old - still gets 1-2k downloads a week :see_no_evil:
matt.fellows
2021-12-05 03:43
My current plan is to get a reliable repro locally, ideally in the core package rather than when in use in Pact JS (less moving parts)
matt.fellows
2021-12-05 03:44
I plan to do this by finding the minimum verifier code and putting it into an integration test in `pact-js-core`
matt.fellows
2021-12-05 03:44
If anybody already has a reliable way of doing this, I?d appreciate that starting point
matt.fellows
2021-12-05 03:50
Next step is to see if the https://github.com/node-ffi-napi/ref-napi/pull/64 Tim updated actually fixes the issue. Again, doing this reliably locally is the first step.
If it does, there are a few options to move forward:
? we release our own version of `ref-napi` with the patch, even if it?s sub optimal (the feedback seemed to be less about it not solving the problem, but more about doing it better/more performant. This is not really something that will impact us in a significant way because it?s a short lived test process, and not running on a production server where resources need to be better managed).
? Find an alternative library (e.g. https://www.npmjs.com/package/sbffi)
? This library looks a lot smaller in scope, and does have some native struct support that might mean we don?t need `ref-napi` (the actual problematic package). It looks a little out of date, but also due to the size and it mostly being written in C it might be a better horse to back if reliable
? Write our own c wrapper that doesn?t use these libraries (that seem to be not actively maintained). This is a little beyond my current mental and time constraints, but definitely an option.
I?m currently leaning toward option 1, because it will enable us to get everything else into shape and unblock @artur and others who are reliant on this quickly, which buys us time to address the technical debt of getting a well functioning FFI package
tjones
2021-12-05 03:53
The `ffi-napi` branch won't actually fix the problems, we'll still get intermittent failures unless the 64 bit integer problem is fixed
matt.fellows
2021-12-05 03:54
Is that relating to this comment https://github.com/node-ffi-napi/ref-napi/pull/64#discussion_r731883193 ? Or just generally, that 64 bit integers won?t work in this package?
tjones
2021-12-05 04:33
yes, that's the comment. The problem is that some pointers won't be correctly transferred
matt.fellows
2021-12-05 05:52
I'm not so sure about the viability of option 2, but it's definitely more desirable!
matt.fellows
2021-12-05 10:08
There are another couple of options:
4) swap out the `ref-napi` types package, which is actually the problematic package (with e.g. https://www.npmjs.com/package/shared-structs), the `ffi-napi` interface doesn?t seem to be problematic.
5) alternative HTTP (or remote) based interface into the core. i.e. not FFI but a similar external interface initiated perhaps by a CLI initialise. This would mean deviating for others, or perhaps is the v2 FFI interface. Also not desirable for obvious reasons.
uglyog
2021-12-05 22:17
The thing I don't understand is why we are using another package to make FFI calls. Node has very good FFI support already.
tjones
2021-12-05 22:18
If you don't use another package, you have to write C code to translate between JS types and the node FFI lib
uglyog
2021-12-05 22:19
No, you don't. I have a working example that calls the FFI functions directly.
tjones
2021-12-05 22:23
I think `ffi` uses the old bindings directly into V8, which aren't stable across node versions. The `NAPI` interface is stable across node versions, but you still have to write C code to connect to a binary lib.
tjones
2021-12-13 06:15
The downside of sbffi is that we can't read structs, but I don't think we are doing that at the moment
tjones
2021-12-13 06:19
Anyway, if we want to change FFI lib, there's actually only one file to change
tjones
2021-12-13 08:19
I spiked sbffi. I haven't got it working yet, but it needs a lot of work (you need custom wrappers for strings, and some of the types (eg pointers) are not actually implemented)
matt.fellows
2021-12-14 05:06
Ah, I got to a really trivial spike and hit a wall with ?how do I read in the string returned from FFI into a node string?
matt.fellows
2021-12-14 05:07
> oh wait, no. We need that for the matching result results, I think?
no we?re mostly good for structs now. Ron made them opaque, so it?s just a pointer that is needed now for the 2 structures that we did need to ?know? about. Complex responses are JSON strings
matt.fellows
2021-12-14 05:08
> Also, the entire buffer for arguments and return values is 1024 bytes.
:disappointed:
matt.fellows
2021-12-14 05:08
So back to option 1 - raw c interface? I could probably manage it, but a few hours of hacky pairing would probably get me a long way. I suppose that is going to require Node Gyp knowledge, so that migth be a pre-requisite for any such pairing
tjones
2021-12-14 05:34
Re: our own C interface, I think we could start with sbffi as the initial implementation. Building it up is mostly reading documentation, I think (for instance, all functions in sbffi assumed little endian byte orders - I don't know if this is reasonable).
tjones
2021-12-14 05:35
Also the documentation for node buffers. We'll want to know *exactly* how those work
matt.fellows
2021-12-14 05:40
> Can we just use that?
I was mostly being silly, but it _is_ a viable alternative. Moving the mock server portion to HTTP (and ideally, a replacement for the Ruby version) would make it easier to migrate for those that haven?t yet
matt.fellows
2021-12-14 05:41
the gRPC framework is for plugins specifically, but no reason it couldn?t be a general standard
matt.fellows
2021-12-14 05:43
On sbffi. That sounds positive then.
I got to the point that I got a BigInt (I think) returned from an FFI function but didn?t know what to do with it. I assumed it was a pointer to the location of a string, but didn?t know how to read that into a NodeJS structure. I then ran out of time
matt.fellows
2021-12-14 05:45
Did you have any intuition about how to wrap strings? (I saw an interface to wrap a string in a buffer for passing *to* the FFI, but not receiving). I?m guessing I?m missing something obvious
matt.fellows
2022-01-02 12:25
OK so back to this. I?m not sure how far `sbffi` will go. I can pass strings to it, but have no idea how to handle strings that are returned from it, it seems to return a BigInt and I?m not sure what to do with it from there. I asked a question on their issues page, so we?ll see - probably simple, but :man-shrugging: . That being said, Tim?s feedback was (essentially) that it is better than `ref-napi` but also probably not going to work across architectures
matt.fellows
2022-01-02 13:31
I?ve done a spike this evening with the n-api interface and can successfully call the `libpact_version` function and print out the string from JS, as well as create a new pact and interaction, passing the opaque pointers about (thanks to Ron for doing that, because almost certainly that would have caused more challenges). I think this is probably the best path forward, until `ref-napi` can fix their bugs (and even then :man-shrugging:). Despite a level of verbosity, we can expose the functions from the c interface fairly straightforwardly to JS and probably without even touching the type definitions in `pact-core`
(It could possibly be less verbose way via the C++ napi interface).
matt.fellows
2022-01-02 13:34
I?ll push up the test repo tomorrow, but some of the things to confirm:
? Will it be portable across OS/architectures?
? How to pull in the correct libraries at install time? (probably will need to bundle all of the dynamic libs, instead of download at `npm install` time for all of the usual proxy issues)
? Could we look at pre-gyp to publish pre-compiled artifacts?
? There are a couple of structs that won?t be easily represented (due to the ref-napi problem), so will probably just need to handle them in c/c++ directly via the n-api. Doesn?t look fatal. The main one I htink we?ll actually use is the multipart function
tjones
2022-01-02 16:03
> Will it be portable across OS/architectures?
This should be pretty straightforward, I think. Would like to see what you've got so far.
tjones
2022-01-02 16:03
> probably will need to bundle all of the dynamic libs,
We already do this in pact-core
tjones
2022-01-02 16:04
> Could we look at pre-gyp to publish pre-compiled artifacts?
Yes, if we're writing our own wrapper, this is the way to go.
tjones
2022-01-02 16:04
> so will probably just need to handle them in c/c++ directly via the n-api.
Let's stay in C if we can
matt.fellows
2022-01-02 20:57
Also the sbffi person responded. TL;DR - don?t return the string, pass a buffer pointer in to the function and have the function write to it. Then we can use the buffer classes directly
matt.fellows
2022-01-02 20:58
I think this is still suboptimal, because i?d still need a c interface to wrap pact ffi to do this, and also the other issues you discovered
matt.fellows
2022-01-02 21:27
I?ll see if I can get a few more hours another night this week away from kids etc.
matt.fellows
2022-01-03 00:49
Added an example invocation of the verifier. Looks straightforward, albeit not sure the reading of the string value is correct
tjones
2022-01-03 07:10
However, `uint32_t` is what's defined in the pact.h header. Is that a mistake, or is it really giving us integer handles?
tjones
2022-01-03 07:13
Anyway, otherwise this looks good! You could add it directly into pact-core if you like - there's a native boundary that abstracts the library from the rest of the code
matt.fellows
2022-01-03 08:14
> However, `uint32_t` is what?s defined in the pact.h header. Is that a mistake, or is it really giving us integer handles?
It is actually an integer handle. Rust retains a reference on the heap from a unique identifier to the actual object, so I believe the typedef is correct. Even down to the differences between the 16/32 bit ones (the pact.h file is auto generated based on the Rust type system)
matt.fellows
2022-01-03 08:15
> Also, maybe we could autogenerate the C code from the header?
That seems plausible-ish, albeit might require more code to auto generate than to write the definitions by hand? Is this something you?ve done before? Initially, I?ll just get the main bits working I think and then once I?ve a good handle on it all we can look at autogen
matt.fellows
2022-01-03 08:16
I quickly shoved the `verify_provider` into the pact core now, and it hangs (it?s within a promise, so I suspect it needs to be made async somehow - which I believe is possible via the napi API)
matt.fellows
2022-01-03 08:17
Also, the other problem I?ve encountered is making the shared library available at runtime in a known location. It wants to use the usual library search paths, I think I?ll need to find a way to use `@rpath` or similar constructs
matt.fellows
2022-01-04 01:54
Does that essentially embed it in the artifact? If so, yes, that'd be ideal
tjones
2022-01-05 10:58
> I'm explicitly linking the library via -llibpact_ffi type thing
That's just telling the compiler to look for a library called `libpact_ffi`. It could be shared or static.
tjones
2022-01-05 19:44
Do you have the static library present? It doesn't seem to be there in the example you posted
tjones
2022-01-05 19:47
A quick google suggests that the library path should be included in the binary at compile time on linux and mac (which means you won't have to tell anything where it is at runtime). What problem are you hitting? Maybe we should have a call and get to the bottom of it.
matt.fellows
2022-01-05 22:25
I?ll have to get a few hours in front of a computer again (still on hols). Strangely, the dynamic library was fine when I was using the `c` based interface, but when I switched to using the C++ one it stopped (because the C++ interface had first class constructs to do async operations, unlike the c interface which looked very complicated and was probably beyond my capabilities at this moment).
I?m hoping it?s just a compiler flag, but :man-shrugging: .
I could embed the static lib, but they are bigger so will make the package even larger.
matt.fellows
2022-01-05 22:25
> library path should be included in the binary at compile time
This is where rpath comes in, right?
uglyog
2022-01-05 22:27
The static library is linked in at link time, and the code will be part of the final binary. The dynamic library is loaded at runtime, and the so/dynlib/dll needs to be present on the users machine at runtime
uglyog
2022-01-05 22:29
The static libraries are bigger because they have to all the code. The linker will remove unused code.
matt.fellows
2022-01-05 22:52
what I?m confused about is why it can?t find the dynamic lib in the C++ code scenario. The library is present in the standard lookup paths, so it should work
matt.fellows
2022-01-05 22:53
also, I should be able to set the rpath to a relative location. I think it might just be a node-gyp setting I?ve got wrong, because looking into the final binary I couldn?t see these things being set (but also, it?s not something I?ve spent a lot of time doing)
matt.fellows
2022-01-05 22:56
For context, the issue I ran into is that the verification (I think ) needs to be made into an async node function so the event loop doesn?t freeze. The consumer side all seemed to work as expected, but the provider side just hangs. i remember this using `ffi-napi` and they have an `async` version of function invocations which does this. I?ve looked at their code, but is not easily transported because it does some other gnarly stuff. It would need to be re-written (in c) or we can just use the convenience methods present in the C++ napi interface which is just a single class with a few methods on it. It took about 10 mins to write (tested without using the shared lib) - except the issue is, of course, when I made a dependency on `libpact_ffi`I ran into this issue
matt.fellows
2022-01-06 01:38
OK I know what it is! There is a header file for C and a different one for CPP
tjones
2022-01-06 09:09
I would definitely avoid C++ if you can. It's mostly personal preference. The biggest reason is that it's much easier to read C in a style that's not your own. C++ can be unreadable to another C++ programmer.
matt.fellows
2022-01-06 09:50
I know C better than C++ but I think it makes sense to go with the code that has the least complexity and supports our needs. I don't have a good handle on that yet, but the C++ libraries are very well supported and widely used, and also reduce a lot of boilerplate (which perhaps code generation could solve). So far the async code is the main reason I could see C being an issue, given how easy it was with the C++ interface.
matt.fellows
2022-01-06 09:51
I also want to make it easy for contributors. At the moment, it is just looking like glue code (and principally I'd like it to stay that way) so the shortest path that doesn't reduce options in the future is where I think I'd like to be
matt.fellows
2022-01-06 13:14
So, I have a https://github.com/pact-foundation/pact-js-core/runs/4727178656?check_suite_focus=true with the NAPI interface integrated for the provider verification run as an async fn. This is obviously spike code (I shoved it into the TS fairly crudely), and still need to deal with the search path business (or make it a static lib). On OSX, we can run the `install_name_tool` over it to set the rpath, strange that it doesn?t seem to search the embedded rpaths in the lib. I think it has something to do with the `.node` file being a bundle and not a true dynamic library, or perhaps the way node does it. In any case, i?m sure it?s easily dealt with.
It was in C++, because I didn?t have it in me to read all of the various libraries I?d need to understand to make it work at that level. We can definitely try it, but I want to get to a position fairly quickly to prove the validity of the approach overall so we can move this forward.
Next week when I?m back at work, I?ll aim to get a similar situation for the consumer side. I?ll be able to compare the C++ interface to the C one I?ve spiked to get a feel for it.
matt.fellows
2022-01-06 22:31
Perhaps this isn?t the best place now (thread is getting long) but I?d like to get some views on whether we should be using https://github.com/nodejs/nan or https://github.com/nodejs/node-addon-api if we go down the C++ route. NAN results in even less code, but it seems bound to v8 directly which means it won?t work across different runtimes. The Node Addon API is a header only library that wraps the Node-API so it is portable across runtimes (e.g. v8, ChakraCore and any others that get developed presumably). I?ve currently taken the path of node-addon-api for now, with the assumption that we want the portability and that a few extra lines of code.
Both are maintained by the NodeJS team anyway.
(you can see https://github.com/nodejs/node-addon-examples#nodejs-addon-examples for a quick summary of differences).
tjones
2022-01-12 04:57
I definitely don't think we should use NAN, because it doesn't use NAPI (which abstracts the JS core from the caller). Node Addon API is the way to go if we want to use C++ (but I still think we should stay in C if possible).
If we want to parse the header file and autogenerate the C code, I think we could use this:
https://www.npmjs.com/package/node-c-parser
tjones
2022-01-12 04:58
If we much use C++ for convenience, then I think we should minimise the amount of C++ features used.
matt.fellows
2022-01-12 05:30
I did see that, albeit I?d have to look deeper to see if useful. It looks more like a lexer than something higher level.
matt.fellows
2022-01-12 11:10
Update: I have a passing build for Mac, Linux and Windows that uses the new C++/Node-API interface: https://github.com/pact-foundation/pact-js-core/runs/4788271016?check_suite_focus=true
matt.fellows
2022-01-12 11:11
From here, my plan is to publish an alpha package and use it in the v3 branch and see if I can get a green build (this will be provider only at this stage, but the bindings are there for the consumer interface.
matt.fellows
2022-01-12 11:11
If this all checks out, we can then revisit this work and make it much more polished, and then get back to merging the consumer branch in we created months ago
matt.fellows
2022-01-13 00:25
green build in Pact JS v3.0.0 on Linux/Windows using a beta version of the core: https://github.com/pact-foundation/pact-js/runs/4796778714?check_suite_focus=true
:tada:
matt.fellows
2022-01-13 00:26
(I had to exclude Node 16, because the old Neon part of the build is still failing for the reasons why we were looking to move in the first place)
tjones
2022-01-27 12:32
@matt.fellows / @yousafn: I ported jest-pact to work with mocha, and published this: https://www.npmjs.com/package/mocha-pact
tjones
2022-01-27 12:34
We have this very old package, https://www.npmjs.com/package/pact-js-mocha , which was compatible with `pact-node@4.5.x` (!!) - should we deprecate it?
yousafn
2022-01-27 12:39
:thinking_face: yeah there are so little users 6-7 a week, its probably worth a dep notice. I think you need to push a new package with a dep notice for it to be picked up in consumers ci, otherwise they only see the notice on the NPM page
yousafn
2022-03-15 01:57
So I have definitely entered the matrix.
1. Wanted to add a PR https://github.com/pact-foundation/pact-js/pull/835 to pact-js but the builds were failing as identified https://github.com/pact-foundation/pact-js/pull/835
2. The root cause, we believe was raised in this issue https://github.com/pact-foundation/pact_broker-client/issues/102 in pact_broker-client and merged https://github.com/pact-foundation/pact_broker-client/pull/103 by Beth and released as https://github.com/pact-foundation/pact_broker-client/releases/tag/v1.59.0
3. This triggers a pact standalone upgrade to https://github.com/pact-foundation/pact-ruby-standalone/releases/tag/v1.88.83
4. This then triggers an update to pact-js-core https://github.com/pact-foundation/pact-js-core/pull/364 which was merged and released as https://github.com/pact-foundation/pact-js-core/releases/tag/v13.5.1 and published to npm as https://www.npmjs.com/package/@pact-foundation/pact-core
a. there is a seperate issue with windows builds here failing, raised https://github.com/pact-foundation/pact-js-core/issues/365
5. Went to update pact-js, and it depends on https://www.npmjs.com/package/@pact-foundation/pact-node npm package which is on v10.17.1 updated 3 months ago.
Now then, what has started off as something simple has ended up being a bit larger.
? The GH repo http://github.com/pact-foundation/pact-node has been renamed to https://github.com/pact-foundation/pact-js-core
? There are 2 npm packages https://www.npmjs.com/package/@pact-foundation/pact-node and https://www.npmjs.com/package/@pact-foundation/pact-core
? pact-js `master` branch uses `pact-node`
Should pact-js use `pact-core` now? or is that on the `v3` branch? :exploding_head:
matt.fellows
2022-03-15 02:19
TL;DR - the mainline version of Pact JS uses the `pact-node` branch of the `pact-js-core` repository.
matt.fellows
2022-03-15 02:20
I?ll brief you on the status of that in the coming days, but it?s back on track now that we have a C++ / node-gyp layer that provides a native interface to Rust
matt.fellows
2022-03-17 09:37
FYI regarding https://github.com/node-ffi-napi/ref-napi/pull/64#issuecomment-1067493234, we have https://github.com/pact-foundation/pact-js-core/tree/spike/node-gyp/native now and are unblocked as far as the v3 migration is concerned. I don?t see us going back to ref-napi any time soon. There are fatal flaws in the design (that were exposed via this issue) that a small PR won?t fix.
Whilst the C++ code looks complex, the reality is once we have a stable integration up and going, it could probably be automated by a conversion script that reads the c++ headers and converts into the boilerplate code.
artur
2022-03-18 11:18
would it be possible to release a new v10 beta to get https://github.com/pact-foundation/pact-reference/pull/186 to pact-js?
matt.fellows
2022-03-18 11:25
Let me check to see if a new rust core release is out. If it?s out, I?ll pull it in and release all the things!
matt.fellows
2022-03-18 11:26
Doesn?t look like the most recent FFI is out. @uglyog I?m happy to release a new FFI, is there anything I should be aware of before doing so?
yousafn
2022-03-21 13:11
Hmm, so I've just added the token for `jest-pact` but the tokens get associated with my user, so publishes from the repo to npm, will have my name attributed, same as your release for `pact-js-core`.
Wonder if we should create the tokens against the original user (which was renamed *mefellows-pact*), so we get consistent authors on the npm packages, when published from CI and we only get a user name if its published from a local machine.
matt.fellows
2022-03-21 22:04
Pretty sure if I re-use `mefellows-pact` it?ll just have a different name there
2022-05-09 17:58
? Welcome to HeyTaco!
yousafn
2022-06-01 12:13
Is there a single place pact specifications (v1-v4) in typescript are defined, `pact-js` perhaps, that TS based projects can consume from?
I know there is a json schema here
https://bitbucket.org/atlassian/pact-json-schema/src/8d0b9d2f38fe517dfbce0e5d0b8755eaefaacb5b/schemas/v2/schema.json#lines-154
I've defined one in pact-msw-adapter, although it's rough and only based on v2
https://github.com/pactflow/pact-msw-adapter/blob/main/src/pactMswAdapter.ts#L371-L407
yousafn
2022-06-01 12:14
It is this PR that got me thinking about it
https://github.com/pactflow/pact-msw-adapter/pull/46#discussion_r886356414
artur
2022-07-05 09:38
@matt.fellows `npm run build` fails for me in this test https://github.com/pact-foundation/pact-js/blob/9aeb4f8a8be583b0003229d44476de6742b26145/src/messageConsumerPact.spec.ts#L40
I think you have changed that area yesterday. Do you still expect that test to pass?
matt.fellows
2022-07-05 09:49
No there are still a bunch of things to do in this branch. Mid way through a refactor to plug the FFI into the existing interface to make upgrading to the 10.x.x package. Once that's done, and we fix any critical issues it can replace the 9.x.x branch and we can stop maintaining two at the same time
matt.fellows
2022-07-05 09:57
Sorry just on mobile. I'd normally branch but I'm actively working on it and don't want any drift / long running branches. This is the problem that I'm trying to solve here!
matt.fellows
2022-07-05 09:57
I'll look at the issue you raised later tonight. Thanks for reporting as always
matt.fellows
2022-07-28 11:31
Don't tell anybody, Alicia wants a proper launch :rolling_on_the_floor_laughing:
matt.fellows
2022-08-01 03:29
Sorry folks :point_up: - a little cleanup of the issues was required :grimacing:
gueorgui
2022-08-11 13:45
Please let me know if there?s anything I can improve. Especially I don?t think there?s any documentation about this function, I don?t think I?d have found out about it if @matt.fellows hadn?t mentioned it here on Slack.
2022-10-31 05:43
This channel will receive notifications from https://github.com/pact-foundation/pact-js for: `issues`, `pulls`, `releases`, `deployments`
2022-10-31 05:43
This channel will receive notifications from https://github.com/pact-foundation/pact-js-core for: `issues`, `pulls`, `releases`, `deployments`
yousafn
2022-12-05 23:07
Tried to merge a couple of PR's in Pact-JS but the ci pipeline isn't happy with the dependencies in the nest-js pact test. Didn't manage to get time to look at it properly today but it is on my radar for tomorrow or this week.
yousafn
2022-12-05 23:08
There has been some activity on the nestjs-pact upgrade to support Pact JS v3, so we might be able to unblock some of that, and then get a nice example consumer and provider repo together showing them off.
matt.fellows
2022-12-05 23:16
> Tried to merge a couple of PR?s in Pact-JS but the ci pipeline isn?t happy with the dependencies in the nest-js pact test. Didn?t manage to get time to look at it properly today but it is on my radar for tomorrow or this week.
I?m likely to remove it, as at the moment it?s not safe to import dynamically via the `file:../../dist` directive. Which makes running it as part of the CI useless as it?s not testing the pushed code - it might as well juts be a standalone project
matt.fellows
2022-12-05 23:16
There are two reasons we run the examples:
1. To show people how to do Pact with common libs
2. As a quasi-regression
As the number of examples grow, using them for (2) is problematic because they overlap a lot in what they actually test, and are quite slow.
matt.fellows
2022-12-05 23:17
So I?m moving move in favour of pulling them out of the Pact JS repo and instead having a proper integration regression suite (probably using something like Cucumber to specify the business requirements, so that other languages can implement and then we have a list of what features are and are not supported)
tjones
2022-12-05 23:19
That?s the merge that broke everything, and it was probably easy to miss because the build was already red
matt.fellows
2022-12-05 23:20
Ah semver. it?s easy to accidentally break stuff though, so I?m not criticising
matt.fellows
2022-12-05 23:21
Still, my point remains about the projects above. We need to be on the latest versions, else the examples are stale and we don?t get the benefit of (1)
matt.fellows
2022-12-05 23:21
having them all in Pact JS means the maintainer of Pact JS needs to keep them up to date.
tjones
2022-12-05 23:21
I didn?t bring nest-js-pact in to pact-foundation because it wasn?t maintained. - simple change requests lived for a long time
tjones
2022-12-05 23:22
There?s some interest and traction on the issue to upgrade it to v10, so that?s positive
matt.fellows
2022-12-05 23:31
I have to drop off for a thing, but we should also discuss V4. V3 was never an API I was very comfortable with but we had it in beta for so long it was all but impossible to change. V4 has some significant changes - plugin support, different interaction models etc.
I?m currently experimenting with a Type State builder pattern for V4 and it feels a lot better and removes a class of problems (what state am I in? Can the user do that just yet? are all of the required fields set?).
See example here: https://github.com/pact-foundation/pact-js/blob/master/examples/v4/plugins/test/matt.consumer.spec.ts (it?s currently dark launched as a beta and behind a feature toggle env var. Side bar: I think I like this way of doing it, because this way there isn?t a long lived branch off the side. Opinions thoughts welcome :slightly_smiling_face:).
I?d like to keep the ?all in one? interface, but it is harder to do when there are more features (you need to guard against state a lot more). I don?t think we even need to keep the interface in V4, or if we do, we can just leave support for what it already does. The newer features are things like synchronous messages and plugins, so you wouldn?t be able to / need to ?migrate? to it anyway
tjones
2022-12-05 23:35
> Side bar: I think I like this way of doing it, because this way there isn?t a long lived branch off the side. Opinions thoughts welcome :slightly_smiling_face:).
YES
tjones
2022-12-05 23:37
``` .usingPlugin({
plugin: 'matt',
version: '0.0.5',
})```
I?m not a fan of this. Can?t it be done at the Pact level, when you initialise the contract?
tjones
2022-12-05 23:37
In general, I think the whole plugin interface exposes too much complexity to the user
tjones
2022-12-05 23:39
From reading that test, I can?t tell what is part of pact, what is plugin boilerplate, and what is part of the interaction description
tjones
2022-12-05 23:40
Also, I have said this before, but the plugin interface should be designed to make things simpler not harder
tjones
2022-12-05 23:43
btw, I read that page, and I still don?t know what I can and can?t do with plugins
tjones
2022-12-05 23:46
Does `startTransport` begin the listener? I think you can avoid the need for this call
tjones
2022-12-05 23:47
Similarly, I don?t think you need to make a distinction here: `addSynchronousInteraction` - Just make PactJS think they?re both asynchronous
tjones
2022-12-05 23:50
`withPluginContents` is the interaction description? I would put the content in there
tjones
2022-12-05 23:51
```pact
.addInteraction('a MATT message')
.interaction(mattMessage, 'application/matt')
.config({ host: HOST })
.executeTest(async (tc) => {
const message = await sendMattMessageTCP('hellotcp', HOST, tc.port);
expect(message).to.eq('tcpworld');
});```
tjones
2022-12-05 23:53
This is very similar to the way I?m doing it:
```setup(
willSendHttpInteraction({
request: {
method: 'GET',
path: '/health',
},
response: { status: httpStatus(['4XX', '5XX']) },
}),
{ port } // config is optional and inherited from root contract
)```
tjones
2022-12-05 23:55
In my (non-Pact) DSL, it would look like:
```setup(
willSendMattMessage({
message: mattMessage,
}),
{ host: HOST }
)```
tjones
2022-12-05 23:56
As long as earlier you did:
``` startContract({
consumerName: 'matt response consumer',
providerName: 'matt response provider',
}).withPlugin(matt)```
tjones
2022-12-06 00:00
You could even ask the plugin ecosystem for the matcher:
```setup(
getMatcher('willSendMattMessage')({
.....
)```
matt.fellows
2022-12-06 01:16
> Does `startTransport` begin the listener? I think you can avoid the need for this call
I?d like to. I think we could put it on the `PactV4` constructor. One challenge is that not all plugins provide transports and they aren?t always going to be used in all interactions.
matt.fellows
2022-12-06 01:16
> Similarly, I don?t think you need to make a distinction here: `addSynchronousInteraction` - Just make PactJS think they?re both asynchronous
That would be very hard to retrofit into the model now I think
matt.fellows
2022-12-06 01:18
> No one asked you, slackbot.
I?m going to delete that entry. Not sure who did it on such a wide string match (hopefully not me!)
matt.fellows
2022-12-06 01:18
> `withPluginContents` is the interaction description? I would put the content in there
No, that accepts the plugin configuration (which is the contents + whatever config the plugin itself takes)
matt.fellows
2022-12-06 01:18
so I think it mostly maps to your expectations, but probably the example is not aiding that
matt.fellows
2022-12-06 01:20
```return pact
.addSynchronousInteraction('a MATT message') // <- I'm testing a req/response message with this description/scenario name
.usingPlugin({ // <- I need a plugin for this interaction
plugin: 'matt',
version: '0.0.5',
})
.withPluginContents(mattMessage, 'application/matt') // <- interaction contents
.startTransport('matt', HOST) // <- use a mock server for this test please
.executeTest(async (tc) => { // <- transport specific test execution. Will validate the mock server was used, and delegate to the plugin mock server
const message = await sendMattMessageTCP('hellotcp', HOST, tc.port);
expect(message).to.eq('tcpworld');
});```
matt.fellows
2022-12-06 01:24
I know what you?re saying about moving the plugin guff up and out of the configuration.
Any ideas on how we could do that, whilst preserving the dynamic nature of the `executeTest` function and also ensuring the right version of a plugin is used? Plugins unfortunately introduce another versioning problem, and we made the decision to be explicit about versioning, rather than implicit (i.e. discovering). We could change that, but we have already seen issues on the provider side where the driver loaded the most recent version of a plugin to verify and it was incompatible with what was used on the consumer side. We fixed that to ensure the same version is used, because you can imagine the classes of bugs we?d need to address if we didn?t.
tjones
2022-12-06 02:30
> That would be very hard to retrofit into the model now I think
Yes. I?m afraid this is my general position on the plugins
tjones
2022-12-06 02:32
> Plugins unfortunately introduce another versioning problem, and we made the decision to be explicit about versioning, rather than implicit (i.e. discovering).
I don?t think they do. I think this problem already exists inside Pact, with matchers.
tjones
2022-12-06 02:34
So, you need some way of saying ?hey, is this matcher understandable by this version of Pact??
tjones
2022-12-06 02:34
At the moment that?s handwaved away by ?pact specification version? but really it?s the same problemn
tjones
2022-12-06 02:35
So, already we need a mechanism for ?do I understand this bit of the contract??
tjones
2022-12-06 02:35
I think it?s important for the plugin framework to reduce complexity, rather than increase it
tjones
2022-12-06 02:37
I don?t know if it will support plugins (it might not need to) but it will be pretty easy to do that
tjones
2022-12-06 02:43
> Any ideas on how we could do that, whilst preserving the dynamic nature of the executeTest function
So, what I?m doing, is there?s a test context that knows all the configuration. This is passed recursively down the matcher tree, and some matchers might write to it. This means that configuration at the matcher (ie, the interaction) level is the same as at the contract level.
> also ensuring the right version of a plugin is used?
I think this is the responsibility of the package manager. When you register a plugin (ie, connect it to Pact) you can maybe set a semver string for the versions that you will accept.
> Plugins unfortunately introduce another versioning problem, and we made the decision to be explicit about versioning, rather than implicit (i.e. discovering).
You need to record the version in the contract, of course, and you need to be able to interpret it at the other end to make sure that your version of the plugin is acceptable.
> We could change that, but we have already seen issues on the provider side where the driver loaded the most recent version of a plugin to verify and it was incompatible with what was used on the consumer side.
This is the fault of the plugin author, yes? You?d need to say what ?breaking change? means, but with semver you could just reject the plugin if you expected it to be incompatible. Yes, it?s a bug if a plugin author violates that, but I mean, this is a good reason not to.
> We fixed that to ensure the same version is used, because you can imagine the classes of bugs we?d need to address if we didn?t.
This feels like it?s unnecessarily strict. I think it?s also not a good look for a contract testing framework to say ?we couldn?t figure out how this contract compatibility should work, so we enforce that you use the same version?
tjones
2022-12-06 02:47
There?s a subtlety in that in order for everything to be a matcher, all matches have to be asynchronous. Since for ?did I receive this request?? - you don?t know yet.
tjones
2022-12-06 02:52
This idea is of everything being a matcher is so powerful. For example, Matchers have `match` functions for testing whether their data matches, and `strip` functions for removing the matchers and returning the example. This means that even `fromProviderState` is a matcher - when asked to match it does an exact match against the data returned from provider state, and when asked to strip, it returns that data.
matt.fellows
2022-12-06 23:12
> This feels like it?s unnecessarily strict. I think it?s also not a good look for a contract testing framework to say ?we couldn?t figure out how this contract compatibility should work, so we enforce that you use the same version? (edited)
I think it did/does support something like that, so we could probably improve that configuration option.
matt.fellows
2022-12-06 23:14
> This idea is of everything being a matcher is so powerful. For example, Matchers have `match` functions for testing whether their data matches, and `strip` functions for removing the matchers and returning the example. This means that even `fromProviderState` is a matcher - when asked to match it does an exact match against the data returned from provider state, and when asked to strip, it returns that data. (edited)
it definitely sounds elegant, but that ship has sailed (at least for this version of Pact). I?d be interested to see how we might be able to adopt/migrate to something like that and any consequences of doing so, but it?s definitely beyond my technical know how to suggest it. Also, it is a bit of a paradigm shift from Pact as it is today and (possibly) it is best to have this _not_ be in Pact, and let the marketplace of ideas choose the winner.
That is to say, it may well be the better paradigm and have it run alongside tools like Pact might be the best way to validate the idea
cc @uglyog who might be keen to learn more about this line of thinking.
matt.fellows
2022-12-06 23:17
> I think it?s important for the plugin framework to reduce complexity, rather than increase it
I?m not sure I agree with this statement, albeit of course in general, reducing complexity usually improves DX and that is a good thing.
What I?m saying is that we don?t actually have a tabula rasa here - we are working with an existing toolchain, ecosystem and user base. These conditions mean we are necessarily constrained in certain choices
matt.fellows
2022-12-06 23:18
> I?m not a fan of this. Can?t it be done at the Pact level, when you initialise the contract?
Coming back to this. I think we probably could and then just behind the scenes add them to the interaction, however as I said, it?s possible different interactions will need different transports (because you might have an HTTP and gRPC or whatever endpoint) so I?m not sure we can work around that `startTransport` fn.
tjones
2022-12-06 23:19
Why not? You can tell from the interaction description. Source: that?s what I did in Case
tjones
2022-12-06 23:27
> an existing toolchain, ecosystem and user base.
That?s true, but the pact file is opaque, so you have a lot of flexibility - unless concepts get surfaced unnecessarily to the user
matt.fellows
2022-12-07 00:55
> That?s true, but the pact file is opaque, so you have a lot of flexibility - unless concepts get surfaced unnecessarily to the user
>
These concepts are already exposed to the user, that's exactly the point.
A case could be made (see what I did there?) That Pact != Pact Specification.
In fact that's really the case, we already know there are many behaviours of Pact that aren't modelled in the spec at all.
Perhaps we should be taking a wider lens of the pact spec to support wider use cases (such as different contract testing solutions?)
tjones
2022-12-07 00:58
From a user perspective, I think pact is the DSL and the concepts that the broker provides
matt.fellows
2022-12-07 00:58
Could you maybe show what you think the Pact DSL could look like with regards to transport specific interactions?
matt.fellows
2022-12-07 01:00
Sorry slack folds old messages and that one I missed (albeit I saw the case ones)
tjones
2022-12-10 12:31
@matt.fellows - this sounds like what I experienced with the tests on master- they would hang at the end for about 30 seconds
yousafn
2022-12-14 15:14
urgh proper yak shaving on this one :point_up_2:
CI builds failing for windows on the tests
CI build to sync with http://docs.pact.io because GH revoked Beth's token a couple days ago, I _believe_.
Haven't got the time to investigate right now. Was just a readme change so the world shouldn't be on :fire-8-bit: :lolsob:
yousafn
2022-12-14 15:15
if it is token and its only in the repo rather than org wide, might be an idea to switch to org wide (considering security implications) - I don't think that func was avail when the builds were initially put in place
2022-12-22 03:28
:white_check_mark: Subscribed to https://github.com/pact-foundation/jest-pact. This channel will receive notifications for `issues`, `pulls`, `commits`, `releases`, `deployments`
2022-12-22 03:28
:white_check_mark: Subscribed to https://github.com/pact-foundation/mocha-pact. This channel will receive notifications for `issues`, `pulls`, `commits`, `releases`, `deployments`
2022-12-22 03:28
:white_check_mark: Subscribed to https://github.com/pact-foundation/nestjs-pact. This channel will receive notifications for `issues`, `pulls`, `commits`, `releases`, `deployments`
tjones
2022-12-23 21:57
@matt.fellows (cc @yousafn) - the reason the pact-js build is brittle is because of rate limits from the github API: https://github.com/TimothyJones/pact-js/actions/runs/3768427742/jobs/6406852374#step:4:68
yousafn
2023-01-09 14:16
Getting loggy with it :point_up_2:
Since the upgrade to pact js v3, we lost the ability to log to a file. This PR adds the ability, but has a few open questions before I'd want to merge it
tjones
2023-03-03 13:55
@matt.fellows: I figured out what was wrong with the windows build for the CLI fork. If you?d like it, this https://github.com/TimothyJones/pact-cli is a CLI-only pact-core - which would mean that the CLI could be removed from pact-core.
yousafn
2023-03-03 14:10
I don't think there are enough tacos to give you for your current productivity levels fella. Thank you!
matt.fellows
2023-03-04 09:23
I reckon Tim, despite his size, could down a whole bunch of tacos given the opportunity
tjones
2023-05-18 01:33
@yousafn: Wouldn't it be better to leave it open if you'd like people to one day implement it?
yousafn
2023-05-18 02:23
for me no, it clogs up my pull request and inbox list, of things raised by me, and if its not going to be actioned, its just noise, No-one has even reacted to the thread, and closing the issue doesn't stop anyone from contributing
tjones
2023-05-18 03:53
Hm. If this is work that the project wants to be done, I reckon it should be tracked somewhere
tjones
2023-05-18 04:38
Or, put another way, I have lots of open tickets. So, if you want me to reopen one, just send it to me :raised_hands:
matt.fellows
2023-05-18 06:03
My view: we should eject this project from the foundation (or we institute a policy that indicates which projects have first class support and those that are "community" supported - I prefer something like this IMO)
We need to discuss how we deal with projects that come in with a blaze of excitement and burn out as fast as they appeared.
No criticism of anyone here, except our process.
tjones
2023-05-18 06:31
I agree. A good start might be a kind of SLA for pact-foundation projects - like a responsiveness commitment (could be months) on PRs and issues. Not necessarily promising to solve all issues, but at least marking them as "would accept a PR that fixes" or "no, we're not doing this"
If an SLA like that were brought in, we could consider it as day zero for all projects
yousafn
2023-05-18 09:37
at least not linked to my pull request or issue text. it wasn?t an ask that came from anyone else, it came from me, and i don?t want to be associated with it anymore
yousafn
2023-05-18 09:39
if you want to create an appropriate issue for tracking, then do so, but it?s not nice to copy someone?s else in entirety tbh
yousafn
2023-05-18 09:41
> Hm. If this is work that the project wants to be done, I reckon it should be tracked somewhere
who wants it to be done?
yousafn
2023-05-18 09:48
i?ve donated other projects to the pact foundation, if this wasn?t my job, i would be really unhappy with having an SLA imposed on me.
i can understand that means a suboptimal experience for users of the project, but it is open source code with no warranty or liability.
tjones
2023-05-18 10:36
I think it?s a good issue to have open- and I think it?s an easy fix (I?ve considered doing it a couple times)
yousafn
2023-05-18 10:36
Then I would re-raise it with the appropriate context with some guide on the work to be done, rather than my random headfart
tjones
2023-05-18 10:38
I wasn?t proposing an SLA to be imposed on the projects, I am proposing an SLA to remain in the pact-foundation. Best applied to new projects on the way in, rather than existing projects in the foundation.
But, I also think that it would be reasonable to say ?hey, we?ll eject this unless it becomes maintained. Here?s what we mean by maintained <etc>?
matt.fellows
2023-05-19 05:10
> how can you SLA an open source project with part time volunteers. hmm
You can?t, really, this is why we are working through the ?SmartBear supported? model. We will be able to put some more SLAs around certain items. I?m using the term SLA loosely here - process is probably the better term.
What I?d like to see is a model that e.g. tags projects and says ?this is community supported, expect XYZ (or expect nothing)? vs ?this is tier 1 project, and you can expect us to behave in ABC? way.
Then we have a defined model about how issues get in, are triaged and categorised, and then communicated about ?what next?.
But in reality, time is what people most want to know, and that?s very hard to do with both open / community volunteers, a small team and a large real estate.
So I?m very much of the view right now
1. Review all of our assets and scrutinise the value we get - I think we should err on the side of brutal
2. Eject projects from the `pact-foundation` into something like `pact-community` (or archive them as the case may be) that are adding little to no value. Spring Cloud might also fit this bucket, so perhaps it?s the first project we use in the model ?it?s not officially supported by Pact or Spring, but we?d like to see XYZ things before it?s moved back to the foundation? (which would include a maintainer that is interested in keeping it alive, a large enough community of users etc.
3. Document and clarify what it means to be an ?official? project
All of this serves us, yes, but it?s about serving our users - it?s unclear from the outside what is real vs a toy, and therefore what can be trusted.
tjones
2023-05-19 05:21
I mean more ?this is the criteria to bring projects in the foundation. Projects that don?t match the criteria might be removed from the foundation?
tjones
2023-05-19 05:21
I think it should be super loose - like, ?maintainers triage issues within 6 months? or something
tjones
2023-05-19 05:22
?don?t want to do that? That?s fine, but we?re not going to accept the request to move it in to pact-foundation?
matt.fellows
2023-05-19 05:34
Yep, totally. We need to of course discuss and agree on all those things but I think we?re saying similar things.
yousafn
2023-07-06 11:50
ahhh what is going on with the release note formatting, also its picked up the feat again, maybe from my merge of master into my branch. Tim, I am sorry!
matt.fellows
2023-07-06 11:54
on the plus side, these are all _easily_ fixible by manually updating the changelog and release in GH. So no harm done really
yousafn
2023-07-13 13:08
Hey hey, I raised an issue for it here
https://github.com/pact-foundation/pact-js-core/issues/456
yousafn
2023-07-13 13:09
it?s fine in draft, but when it gets converted to a published release, it?s missing the new lines, and not marking as latest
yousafn
2023-07-13 13:14
I?m only doing the minimal to provide the new lines and marking as latest, I didn?t want to change up the release pipeline too much from its original good state you put it in.
Happy for you to raise a ticket to track for updating to release-please. I am off in the land of go, python and php at the moment.
tjones
2023-07-13 13:39
I don?t think it needs a ticket, it?s just if you?re implementing changes to the pipeline it?s a nice option as it does many of the things I had to build
yousafn
2023-07-13 13:46
unless its a drop in replacement, I?d have to go read docs, and it exceeds the scope of ensuring we have some new lines and mark it as latest, so I would want something to track it. I?d want to test it out e2e on my own fork, so I personally, if your asking me to do it while I?m there, I?d want a ticket.
yousafn
2023-07-13 13:47
I?ve already implemented the big raft changes to the pipeline in the prebuild pr, but had a couple of minor snags
