Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Major refactor: TypeScript, named API arguments, access to native fetch, etc. #200

Closed
wants to merge 54 commits into from

Conversation

jmrog
Copy link
Contributor

@jmrog jmrog commented Sep 24, 2019

This PR will allow us to finally release a v2 of stardog.js. I'm putting it up now mainly for discussion, though if we decide that it's mergeable as-is, that's fine too. I'll try to highlight the major changes.

  1. TypeScript instead of JavaScript. This allows us to:
    i. auto-generate the documentation,
    ii. prevent the types from falling out-of-sync with code, as they have before,
    iii. (hopefully) have a better development experience overall. The repo enforces noImplicitAny, so types are everywhere. This will close Move to TypeScript #161.
  2. Named API arguments, instead of functions with many unnamed parameters, where order matters and you have to remember which is which, etc. This will close API change: from arbitrary function arguments to objects with named properties #159.
  3. Standardization around import instead of require for importing modules. This aligns stardog.js more with the overall ecosystem, and prevents having to switch code styles as much between our repos. This will close Fix tests when using Stardog 6.0.1 #183.
  4. The native fetch results are now exposed directly to the consumer. This will help us to stop adding hacks to stardog.js to aid Stardog Studio, where we sometimes need those results. (For users who still want the convenience of a transformed response, the httpBody method is now exposed on utils; we may also want to add a config setting that will always apply that transform, if the user desires).
  5. Hard dependencies on fetch polyfills are removed, though of course they may be added. This also lets us avoid a bunch of hacks in Stardog Studio (e.g., when we want to fetch in Electron); the same hacks will be unnecessary for other consumers.
  6. The code is made much cleaner (IMO) and reduces a lot of boilerplate. It becomes quite obvious that stardog.js is mostly just a wrapper around fetch, tailor-made for Stardog. As one example, this:
const getAll = (conn, database) => {
  const headers = conn.headers();
  return dispatchDBOptions(conn, {
    headers,
    database,
    method: 'GET',
  })
    .then(httpBody)
    .then(res => {
      if (res.status === 200) {
        return Object.assign({}, res, {
          body: flat.unflatten(res.body),
        });
      }
      return res;
    });
};

becomes this:

export const getAll = ({ connection, database }: Parameters<typeof get>[0]) =>
  get({
    connection,
    database,
    optionsToGet: null,
    method: RequestMethod.GET,
  });

Just about all of the source code consists of exports of wrappers like this.
7. Via TypeScript, tons of "magic string" parameters are now enum members (e.g., RequestMethod, ContentType, RequestHeader, etc.). This provides both type safety and more autocompletion.

Together, these changes also close #194.

There are probably other relevant things that I'm not thinking of right now. The most painful part of all this is likely to be the named argument changes, which will require lots of (albeit trivial) changes in Stardog Studio. But we can address that when the time comes.

Feel free to try this branch out, fire away with comments, pain points, etc.

jmrog and others added 30 commits April 10, 2018 15:14
* Upgrade fetch ponyfill

* Get tests passing again after upgrading fetch-ponyfill
* Update to version 6 and fix CircleCI (#185)

* add full string values for test expectations

* fix tests and update version note in README for stardog 6

* attempt to fix circleci by updating to new jfrog url

* explicitly test graphql explain support (#184)

* Add icv.report and icv.reportInTransaction functionality (close #187)

* Add types for new ICV methods and fix types for existing (close #186)

* Add tests for new ICV methods
@@ -65,7 +65,7 @@ After releasing, be sure to push to master, including the tags (so that the rele

## Version details

The current version of stardog.js has been tested against version 5.2.0 of Stardog. You are encouraged to use this library if you are using version 5 or greater of Stardog. However, there is very little code that is version specific in stardog.js. It is essentially just a convenience wrapper around `fetch`. It is very likely that many of the exposed methods will work on older versions of Stardog, but this has not been tested.
The current version of stardog.js has been tested against version 6.2.0 of Stardog. You are encouraged to use this library if you are using version 5 or greater of Stardog. However, there is very little code that is version specific in stardog.js. It is essentially just a convenience wrapper around `fetch`. It is very likely that many of the exposed methods will work on older versions of Stardog, but this has not been tested.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything to be said about Stardog 7?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based on the dockerfile, aren't the tests being run against 7 now?

PUT = 'PUT',
}

export const enum ResponseStatus {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow I feel like more values here will be needed...

database,
fileName,
fileContents,
}: BaseDatabaseOptionsWithFileName & {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a dynamic type instead of something also defined in ../types? Is it because this is the only place it's used? Full disclosure: I know very little about TypeScript or the conventions thereof.

requestHeaders: {
[RequestHeader.ACCEPT]: ContentType.TEXT_PLAIN,
},
pathSuffix: `${database}/docs/size`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to get REALLY crazy and store HTTP path constants like "docs" in an enum? Stardog does it (in a sense), but it could be seen as dramatic overkill...


const dispatchFetchWithGraphUri = getFetchDispatcher({
allowedQueryParams: ['graph-uri'],
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen this method a couple times now (on graph.ts too, for instance). Would it make any sense to move it to request-utils and import as needed?

import { RequestHeader, ContentType, RequestMethod } from '../constants';
import { BaseDatabaseOptions, BaseDatabaseOptionsWithGraphUri } from '../types';

// TODO: Confirm whether this is really necessary.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like maybe not, since you're able to set it in dispatchFetch. Based on the name and my memory I'm pretty sure I wrote it, but can't remember why any more.

@@ -0,0 +1,109 @@
/**
* @module stardogjs.db.versioning
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last I knew this was going away as of Stardog 7. Check with Evren to make sure this should even bother being here now

// flatten everything down into a single string
.replace(whitespaceMatcher, '')
.toLowerCase();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these take case into consideration??

name: string;
}

export type Action =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have upper and lowercase here?

@@ -1,6 +1,6 @@
{
"name": "stardog",
"version": "1.4.0",
"version": "2.0.0",
"description": "Stardog JavaScript Framework for node.js and the browser - Develop apps using the Stardog RDF Database & JS.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking is almost certainly going to want to change this from "Stardog RDF Database"

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