refactor: move createFromString and resource from Prism Http to CLI #1009

XVincentX · 2020-02-27T23:52:39Z

This PR is a proposal of moving the methods createClientFromResource and createClientFromSpec from the prism-http package to prism-cli package

The main rationale behind this is that the dependencies of such feature (json-ref-resolver, json-ref-readers) have an hard dependency on the fs package that makes prism-http a little bit harder to embed in the browser (you need to configure web pack to mock the fs module to empty)

This is technically a breaking change since we're removing a function although we have never "marketed" our client API — I'm summoning @philsturgeon to get his opinion.

If we're going to take this as a breaking change (Prism 4), then I'd take the occasion to also

Change our artefacts directory from dist to lib — it makes more sense and does not make you feel bad when importing stuff // cc @marcelltoth — we had such discussion some time ago
Rename some of the client methods to something that makes a little bit more sense

Closes #1008
Closes stoplightio/elements/issues/261

philsturgeon

It looks like a change is happening to http-client which is actually something we’ve advertised as useable in the docs. If documented package API is changing then it’ll need to be a major version.

I don’t have many opinions on heather you should bundle other major changes in here. It does make sense to do various necessary breaks in one go, but keep in mind hosted mocking will also need to change and that’s need more QA time, and they’re already fairly busy. If you consider that sort of stuff and make a decision then I support it either way! 🙌🏻

docs/guides/http-client.md

Co-Authored-By: Phil Sturgeon <[email protected]>

packages/http-server/src/__tests__/server.oas.spec.ts

karol-maciaszek

The is a movement in a good direction!

Since we somehow have a green light to break things why not move getHttpOperations(string) to http-spec? The spec type detection (which will be more and more complex over time, see https://github.com/stoplightio/prism/blob/feat/postman-collection/packages/http/src/getHttpOperations.ts#L48) is a good candidate for being shared.

The only problem I see with such an approach is that a folk wanting to put some operations programmatically into prism will need to import http-spec package. But we can do it for him and reexport getHttpOperations in prism. Thoughts?

docs/guides/http-client.md

packages/http-server/src/__tests__/server.oas.spec.ts

packages/http/package.json

philsturgeon

I don't think I said "green light to break things" haha just, please use cautious and common sense. No leroy jenkins here.

packages/http-server/src/__tests__/server.oas.spec.ts

XVincentX · 2020-02-28T17:07:40Z

Since we're going to ship this as a breaking change I think I will tweak a little bit the function names (the current one might not exactly make that much sense) as well as maybe rearrange the parameters. I'll look into this later today and put something online.

marcelltoth · 2020-02-28T17:10:50Z

Change our artefacts directory from dist to lib — it makes more sense and does not make you feel bad when importing stuff // cc @marcelltoth — we had such discussion some time ago

Just my two cents here. I think if we are going to need a major bump we should take the time to really define what our public API is. Merely renaming the directory to lib and saying it's OK to import from there is disastrous in my opinion. From that point on changing any export in any file will technically be a major break. I'm sure http\dist\mocker\negotiator\internalHelpers shouldn't be part of a public library distribution. But probably http-server/getHttpConfigFromRequest neither.

Let's take a step back, decide what the goal is for each of the packages (in terms of external use), and what are the key APIs we'd like to offer to the public to achieve them.

Then put these files into somewhere (lib folder, index.ts export / top level files / whatever we decide on), keep the rest where it is, and clearly document what is part of our API and what is not.

XVincentX · 2020-02-28T20:58:30Z

@marcelltoth I understand where you're coming from and I would love to do that, but I do not think we can now. The public JS API will probably be done (if done) once we'll gather from more feedback from hosted Prism, Mocking Tab usage and all these product using the package.

On the other hand, we need to make Prism Http browser-compatible as soon as possible since it's blocking some stuff we're doing — and I took the occasion of the breaking change to do propose some other reorganisation — among these the idea of using lib directory as output instead of dist because it just makes us feel sick. I'm fine deferring this if it's an issue for anybody though.

marcelltoth · 2020-03-02T12:44:18Z

@XVincentX Then I propose to keep it that way and import from dist as before.

That pushes the "risk" to the consuming packages if they import from dist. Then we sort it out in the next major version, and everyone's happy.

However if we publish a major version where everything is public, how are we going to support that? Do we release a new major every 2 days? Do we break semver? In my eyes either of these are worse than just keeping it in dist and surviving with those ugly imports until we have the time to create an actual long term solution.

XVincentX · 2020-03-02T16:11:37Z

We'll keep the dist directory as it is at the moment — I am fine deferring the discussion when we'll have the time.

That said, we still need this change for our platform and so apparently this change is unenviable unless we publish this as a 4 beta and use it internally, which might be ok with me and with platform.

marcelltoth

I still have two concerns:

Publicly teaching users that it is OK to import stuff from /dist is not great. It'd be OK if we bundled stuff but as we are not, this makes it very difficult to define correct semver versioning.
importing a package called cli feels weird. A cli feels like it is intended to be an end-user thing that you run, not something you use in other packages, right?

I understand that this is something you want to work on later separately however, so this one is good to go. It is definitely a great improvement and does what it promises.

marcelltoth · 2020-05-08T08:33:08Z

packages/http/src/__tests__/http-prism-instance.spec.ts

@@ -4,7 +4,7 @@ import { Scope as NockScope } from 'nock';
 import * as nock from 'nock';
 import { basename, resolve } from 'path';
 import { createInstance, IHttpProxyConfig, IHttpRequest, IHttpResponse, ProblemJsonError } from '../';
-import { getHttpOperationsFromResource } from '../getHttpOperations';
+import { getHttpOperationsFromResource } from '@stoplight/prism-cli/src/operations';


Is /src/ in the bundle? Should it?

In the example (README.md) you are importing it from /dist/ which looks more appropriate.

This applies to ~10 other places as well.

packages/http/src/client.ts

packages/http/src/mocker/generator/JSONSchema.ts

docs/guides/http-client.md

karol-maciaszek

Looks good 👍

packages/http/src/mocker/index.ts

XVincentX · 2020-05-08T16:52:28Z

@marcelltoth

I'll address all the comments here:

Publicly teaching users that it is OK to import stuff from /dist is not great. It'd be OK if we bundled stuff but as we are not, this makes it very difficult to define correct semver versioning.

That is very correct, and it was a mistake, I admit it. We aren't going to release this breaking change soon; most likely we'll be still on 3.x for a while. The idea is to accumulate all the breaking changes (included your very good ideas on defining a public API) and then release. So at the moment, no harms has been done

importing a package called cli feels weird. A cli feels like it is intended to be an end-user thing that you run, not something you use in other packages, right?

It is a bit weird, but I really do not see any good reason to create a prism-utils package. If we can find another part where we can paste this, I'm all for it. Maybe http-spec? I have no clue

Is /src/ in the bundle? Should it?

Eheheh ok this is complicated — essentially is a trick to use TypeScript in VSCode without having to compile the code first. I agree that's not great, but

It's in the tests — not shipped code
We can fix that by exporting stuff from the main package (which unfortunately though has some side effects, so not viable right now)

In stoplightio#1009 the getHttpOperationsFromSpec moved to the prism-cli package. The documentation needs to be updated accordingly

…#1820) * Update Http-server doc with new location of getHttpOperationsFromSpec In #1009 the getHttpOperationsFromSpec moved to the prism-cli package. The documentation needs to be updated accordingly * Update packages/http-server/README.md Co-authored-by: Thomas Pytleski <[email protected]>

XVincentX added 6 commits February 27, 2020 17:44

refactor: move createFromString and resource to CLI

c4bf34e

fix: remove import

eb48cc7

docs: clarify the movement

b709659

test: fix

6985bd5

chore: move package to cli

3f1d04b

docs: update

6d3ec45

XVincentX requested review from karol-maciaszek, marcelltoth and philsturgeon February 28, 2020 00:06

fix: correct the deps

2cfd601

philsturgeon reviewed Feb 28, 2020

View reviewed changes

docs/guides/http-client.md Outdated Show resolved Hide resolved

docs/guides/http-client.md Outdated Show resolved Hide resolved

Apply suggestions from code review

098256c

Co-Authored-By: Phil Sturgeon <[email protected]>

philsturgeon reviewed Feb 28, 2020

View reviewed changes

packages/http-server/src/__tests__/server.oas.spec.ts Outdated Show resolved Hide resolved

karol-maciaszek reviewed Feb 28, 2020

View reviewed changes

docs/guides/http-client.md Outdated Show resolved Hide resolved

docs/guides/http-client.md Outdated Show resolved Hide resolved

packages/http-server/src/__tests__/server.oas.spec.ts Outdated Show resolved Hide resolved

packages/http/package.json Outdated Show resolved Hide resolved

philsturgeon reviewed Feb 28, 2020

View reviewed changes

packages/http-server/src/__tests__/server.oas.spec.ts Outdated Show resolved Hide resolved

XVincentX added 2 commits February 28, 2020 11:10

refactor: rename function name

71e926d

refacotr: rename file

70d5e7a

refactor: renaming completion

805af10

XVincentX force-pushed the feat/adios-fromSpec branch from cbbc1b8 to 805af10 Compare February 28, 2020 17:12

XVincentX added 3 commits March 13, 2020 10:15

Merge branch 'master' into feat/adios-fromSpec

bf3a6cd

canary

d8c3608

call it canary

ced29e3

XVincentX force-pushed the feat/adios-fromSpec branch 2 times, most recently from 9c115c8 to 4894178 Compare March 13, 2020 15:50

test: rename

dcb3f78

remove refs

6f488e4

XVincentX force-pushed the feat/adios-fromSpec branch from 98414a7 to 6f488e4 Compare May 7, 2020 16:43

XVincentX added 3 commits May 7, 2020 11:44

import only us locale

1866fa7

type is import revisit

071b9c5

prefer types

44e93c0

XVincentX force-pushed the feat/adios-fromSpec branch from 6eb0688 to 71f6fc1 Compare May 7, 2020 16:55

change stuff

08d82f4

XVincentX force-pushed the feat/adios-fromSpec branch from 71f6fc1 to 08d82f4 Compare May 7, 2020 16:55

add back deps

168cb80

XVincentX force-pushed the feat/adios-fromSpec branch from 9e8816b to 168cb80 Compare May 7, 2020 17:02

XVincentX marked this pull request as ready for review May 7, 2020 17:14

call beta again

90d60b2

XVincentX force-pushed the feat/adios-fromSpec branch 3 times, most recently from 8cd51a4 to 90d60b2 Compare May 7, 2020 17:43

XVincentX requested a review from karol-maciaszek May 7, 2020 17:47

XVincentX added 2 commits May 7, 2020 12:49

move test file

4bc6e85

fix import

133696c

marcelltoth approved these changes May 8, 2020

View reviewed changes

karol-maciaszek approved these changes May 8, 2020

View reviewed changes

packages/http/src/mocker/index.ts Show resolved Hide resolved

XVincentX added 4 commits May 8, 2020 11:15

rename typeis usage

f68f30c

reword docs

e9629f2

default export

f597c6c

docs: changelog

5a9870b

XVincentX merged commit 15db66a into master May 8, 2020

XVincentX deleted the feat/adios-fromSpec branch May 8, 2020 16:52

dnascimento mentioned this pull request May 3, 2021

Update Http-server doc with new location of getHttpOperationsFromSpec #1820

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: move createFromString and resource from Prism Http to CLI #1009

refactor: move createFromString and resource from Prism Http to CLI #1009

XVincentX commented Feb 27, 2020 •

edited

Loading

philsturgeon left a comment

karol-maciaszek left a comment

philsturgeon left a comment

XVincentX commented Feb 28, 2020

marcelltoth commented Feb 28, 2020

XVincentX commented Feb 28, 2020

marcelltoth commented Mar 2, 2020

XVincentX commented Mar 2, 2020

marcelltoth left a comment

marcelltoth May 8, 2020

karol-maciaszek left a comment

XVincentX commented May 8, 2020

refactor: move createFromString and resource from Prism Http to CLI #1009

refactor: move createFromString and resource from Prism Http to CLI #1009

Conversation

XVincentX commented Feb 27, 2020 • edited Loading

philsturgeon left a comment

Choose a reason for hiding this comment

karol-maciaszek left a comment

Choose a reason for hiding this comment

philsturgeon left a comment

Choose a reason for hiding this comment

XVincentX commented Feb 28, 2020

marcelltoth commented Feb 28, 2020

XVincentX commented Feb 28, 2020

marcelltoth commented Mar 2, 2020

XVincentX commented Mar 2, 2020

marcelltoth left a comment

Choose a reason for hiding this comment

marcelltoth May 8, 2020

Choose a reason for hiding this comment

karol-maciaszek left a comment

Choose a reason for hiding this comment

XVincentX commented May 8, 2020

XVincentX commented Feb 27, 2020 •

edited

Loading