[ZL-796] - Support Rest API migration #42

Varomir · 2021-02-04T16:53:09Z

Ticket: ZL-796

Description

This PR changes how the JS-SDK handles API versions, to support easy migration from the legacy API Vapid to the new versioned API Rest. Read more about the Rest versioned API in our: Confluence pages.

An optional version can now be passed from the library client-side or default rest/v1.0 will be applied.

The API version can be specified at the time of the request.

Example	Requested URL
`procore.get({base: '/me'})`	`https://app.procore.com/rest/v1.0/me`
`procore.get({base: '/me', version: 'v1.1'})`	`https://app.procore.com/rest/v1.1/me`
`procore.get({base: '/me', version: 'vapid'})`	`https://app.procore.com/vapid/me`

lib/client.ts

package.json

lib/client.ts

…core/js-sdk into Remove_Dependencies

… default route

Varomir · 2021-02-10T06:54:10Z

lib/client.ts

+    return url;
+  };
+
+  private version = (apiVersion: string = 'v1.0'): string => {


Do we need to process '' (passed empty string from the client code) as default behaviour (v1.0) or we should throw an error in this case? (as now)

Varomir · 2021-02-10T11:55:48Z

lib/client.ts

+
+  private url = (endpoint: Endpoint): string =>
+    notNil(endpoint) && (endpoint.constructor === String || endpoint instanceof String) ?
+      `${this.host}${endpoint}` : // TODO: Do we want to allow this. Version will not be handled for this


This is probably one of the main questions for code review.

My vision: JS-SDK should not support the endpoint instanceof String login anymore

Varomir · 2021-04-06T12:22:41Z

LGTM

…ders being sent on all requests

bpross · 2021-05-26T17:46:40Z

@njbbaer @gilmcquillan updated to send the sdk version in the headers. Let me know what you think about the naming.

chrisarevir · 2021-02-19T23:15:59Z

package.json

  },
  "author": "Procore Tech <insights@procore.com> (http://procore.com)",
-  "license": "ISC",
+  "license": "MIT",


nb: Might actually be Procore Development License, see here, but that's a topic for legal I think

Interesting. Taking this PR over, so a bit unsure about this change. @Varomir @gilmcquillan any reason that the license here changed?

Given that the rest of our open source libraries use MIT, I think we should use that here as well.

Ruby SDK, MIT: https://github.com/procore/ruby-sdk/blob/master/LICENSE.txt
Blueprinter, MIT: https://github.com/procore/blueprinter/blob/master/MIT-LICENSE
Sift, MIT: https://github.com/procore/sift/blob/master/MIT-LICENSE

I will open up a discussion with legal around what license we should be using.

package.json

CHANGELOG.md

…thod

lib/client.ts

Varomir · 2021-06-03T12:30:42Z

@bross I've got a compilation error after yarn & yarn test

TSError: ⨯ Unable to compile TypeScript:
test/authorize.spec.ts:2:27 - error TS2307: Cannot find module '../dist/index' or its corresponding type declarations.

2 import { authorize } from '../dist/index'
                            ~~~~~~~~~~~~~~~

js-sdk % node --version
v10.22.1
js-sdk % npm --version 
6.14.6

bpross · 2021-06-03T15:31:20Z

@bross I've got a compilation error after yarn & yarn test

TSError: ⨯ Unable to compile TypeScript:
test/authorize.spec.ts:2:27 - error TS2307: Cannot find module '../dist/index' or its corresponding type declarations.

2 import { authorize } from '../dist/index'
                            ~~~~~~~~~~~~~~~

js-sdk % node --version
v10.22.1
js-sdk % npm --version 
6.14.6

Thanks for pointing this out @Varomir

After some investigation, this appears to be a side effect of importing package.json to get the SDK version. We have two options here: we can either modify the tests to reference the new location dist/lib/* or create a new file that stores the SDK version and read from that. Will discuss in standup.

… use this directory since we are importing the version from package.json and the path is relative, so the compiled structure changes

package.json

…core/js-sdk into ZL-796-update-Procore-JS-SDK-to-Rest

Varomir added 4 commits February 4, 2021 18:35

[ZL-796] - Remove unused import

27e6219

[ZL-796] - Remove dependency to the '@procore/js-sdk-endpoints'

4a511c2

[ZL-796] - Add 'api_version?' optional parameter

0e20ce3

[ZL-796] - Bump 'js-sdk' version to '2.5.0'

48354a8

Varomir requested review from brock-noah, davismartin, gilmcquillan and njbbaer February 4, 2021 16:53

Varomir self-assigned this Feb 4, 2021

Varomir commented Feb 4, 2021

View reviewed changes

lib/client.ts Outdated Show resolved Hide resolved

Varomir commented Feb 4, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

Varomir commented Feb 4, 2021

View reviewed changes

lib/client.ts Outdated Show resolved Hide resolved

miromyke reviewed Feb 4, 2021

View reviewed changes

lib/client.ts Outdated Show resolved Hide resolved

lib/client.ts Outdated Show resolved Hide resolved

lib/client.ts Outdated Show resolved Hide resolved

lib/client.ts Outdated Show resolved Hide resolved

lib/client.ts Outdated Show resolved Hide resolved

gilmcquillan and others added 9 commits February 5, 2021 17:19

Remove dependencies and improve tests

21a71b4

[ZL-796] - Fix failed test with 'undefined' apiVersion parameter

0a2db71

[ZL-796] - Remove unused import

7c0bd30

[ZL-796] - Bump major version

cecbec8

Fix yarn compile

8226274

Merge branch 'ZL-796-update-Procore-JS-SDK-to-Rest' of github.com:pro…

1a4929d

…core/js-sdk into Remove_Dependencies

[ZL-796] - Revert 'client.spec.ts' to the original state

230083c

[ZL-796] - Add 'apiVersion' specific tests

5dacf1a

[ZL-796] - Cosmetic update test: switch from '/vapid' to '/rest/v1.0'…

778a77d

… default route

Varomir commented Feb 10, 2021

View reviewed changes

Varomir and others added 6 commits February 10, 2021 17:53

[ZL-796] - Update README

7510cc4

[ZL-796] - Add changelog record

2ef888a

More updates

3a53bb5

More updates

f751068

Add client options

ebe4dae

Improve tests

410e146

drymar approved these changes Mar 31, 2021

View reviewed changes

gilmcquillan added 2 commits March 31, 2021 17:02

Eliminate breaking changes and add tests for refresh and revoke.

f7adfc5

Update the Changelog with Breaking Changes section.

97f6a93

davismartin approved these changes Apr 1, 2021

View reviewed changes

Start sending sdk version in all requests. Update tests to verify hea…

30f6950

…ders being sent on all requests

chrisarevir reviewed May 26, 2021

View reviewed changes

gilmcquillan reviewed May 27, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

Ben Ross added 2 commits May 27, 2021 09:50

Rollback unnecessary changes to package.json and yarn.lock.

7772077

Fix issue with string formatting

faf84db

gilmcquillan reviewed Jun 2, 2021

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

gilmcquillan reviewed Jun 2, 2021

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

Update changelog to include new header as well as renaming destroy me…

c7e50a9

…thod

gilmcquillan reviewed Jun 2, 2021

View reviewed changes

lib/client.ts Outdated Show resolved Hide resolved

Drop X in Sdk header

1cb012b

Change main to use dist/lib and update tests to use the same. Need to…

0ae0639

… use this directory since we are importing the version from package.json and the path is relative, so the compiled structure changes

njbbaer reviewed Jun 7, 2021

View reviewed changes

package.json Show resolved Hide resolved

Ben Ross and others added 6 commits June 7, 2021 10:46

Make sure to include dist/lib folder when publishing package

a0ba968

Fix json syntax error

1583fa9

More updates to dependencies and updated yarn.lock file.

8e5ff29

Break up the sdk header into version and language

26dbc89

Merge branch 'ZL-796-update-Procore-JS-SDK-to-Rest' of github.com:pro…

783b2fb

…core/js-sdk into ZL-796-update-Procore-JS-SDK-to-Rest

Update change log to include new header

26f6db0

gilmcquillan approved these changes Jun 10, 2021

View reviewed changes

bpross merged commit 9f6a4bb into master Jun 11, 2021

bpross deleted the ZL-796-update-Procore-JS-SDK-to-Rest branch June 11, 2021 16:23

gilmcquillan mentioned this pull request Oct 3, 2022

New CSRF strategy #41

Closed

[ZL-796] - Support Rest API migration #42

[ZL-796] - Support Rest API migration #42

Uh oh!

Conversation

Varomir commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket: ZL-796

Description

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Varomir Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

Varomir Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

Varomir commented Apr 6, 2021

Uh oh!

bpross commented May 26, 2021

Uh oh!

chrisarevir Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

bpross May 27, 2021

Choose a reason for hiding this comment

Uh oh!

bpross May 27, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Varomir commented Jun 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bpross commented Jun 3, 2021

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Varomir commented Feb 4, 2021 •

edited

Loading

Varomir commented Jun 3, 2021 •

edited

Loading