39 - Module restructuring and rearchitecture

[GPortas]

What this PR does / why we need it:

This PR completely restructures the module, based on a new architecture design.

The code now only includes a single Dataverse API operation, which serves as an end-to-end architecture test, thus losing the support for the operations offered in the old package. These operations, among others, will be implemented in the upcoming PRs before releasing the package, to make the next release version (2.0.0) backwards compatible with the 1.x.x versions.

Which issue(s) this PR closes:

• Closes Create a npm module for a Dataverse API client so the frontend application and other community applications can use it dataverse-frontend#39

Special notes for your reviewer:

Testing

Includes both unit tests and an integration test related to the added functionality. The integration test is very basic and currently points to demo.dataverse.org. I have added a TODO mentioning that it is necessary to establish a suitable environment for integration tests (Idea: we could setup that environment with containers).

Different test suites have been configured to be executed all together, or independently, depending on the test nature (unit or integration), so there should be no problem in including only unit tests execution in future code validation mechanisms, such as hooks or PR checks. You can find more information about this in the README.

Docs

The old docs, based on TypeDoc, have been removed along with the old code. In order not to extend this PR, a specific issue will be created to address the documentation of the module, since by having to recreate the documentation from scratch and given the age of the old documentation, it may be a good opportunity to explore other options for documenting.

Suggestions on how to test this:

Tests and linting

There are instructions in the README on how you can run the different types of tests, coverage, as well as other linting and formatting related operations.

Testing the functionality

As mentioned, the restructured package only covers one functionality at the moment, which is retrieving the Dataverse version.

To test this functionality we have to import the npm package from a test application. Usually we would install the package from the npm registry, but since it is a development unpublished version, we have to reference a pre-built local package from the test application.

To build the package locally:

• 1. Install dependencies omitting dev: npm install --omit=dev.
• 2. Build the package code: npm run build.
• 3. Create the .tgz local package: npm pack.

In dataverse-frontend/39-js-dataverse-test there is an example of how the local package is imported and configured and a call to "getDataverseVersion" is made. In this example we can see:

• https://github.com/IQSS/dataverse-frontend/blob/39-js-dataverse-test/package.json#L105 - The package.json dependency setup
• https://github.com/IQSS/dataverse-frontend/blob/39-js-dataverse-test/src/sections/hello-dataverse/HelloDataverse.tsx#L11 - Call to getDataverseVersion from the UI
• https://github.com/IQSS/dataverse-frontend/blob/39-js-dataverse-test/index.html#L35 - API base URL environment variable setup

Is there a release notes update needed for this change?:

N/A

Additional documentation:

• js-dataverse old code base analysis: link

[pdurbin]

Overall, this looks great! I'm excited about the plan to simply create a 2.0.0 version from this repo. I left some comments.

[pdurbin]

First of all, it's a bit confusing to me to see multiple ways of running the same command. I don't have a strong preference between yarn and npm. Can we pick one? I realize that this a pre-existing condition from the old README! 😄

When I run yarn install it even tells me I should pick one:

It is advised not to mix package managers in order to avoid resolution inconsistencies caused by unsynchronized lock files.

Full output:

$ yarn install
yarn install v1.22.19
warning package-lock.json found. Your project contains lock files generated by tools other than Yarn. It is advised not to mix package managers in order to avoid resolution inconsistencies caused by unsynchronized lock files. To clear this warning, remove package-lock.json.
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 🔨  Building fresh packages...
success Saved lockfile.
✨  Done in 7.24s.
$ 

As I go through the README I guess I'll use yarn since it was listed first but in the frontend repo we are using npm so maybe npm should be used instead for consistency.

[GPortas]

I agree. I kept both commands from the old README, but it's better to be consistent with npm, as you say.

[pdurbin]

yarn lint found one problem:

$ yarn lint
yarn run v1.22.19
$ eslint src --ext .js,.ts

/Users/pdurbin/github/iqss/dataverse-client-javascript/src/core/domain/useCases/UseCase.ts
  2:20  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

✖ 1 problem (0 errors, 1 warning)

✨  Done in 1.79s.
$ 

(yarn lint:fix didn't fix it by the way.)

[GPortas]

It is a well-detected warning, following what the typescript-eslint plugin mentions in https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-explicit-any.md.

In my opinion, the use of "any" is detecting, which is the only one in the code, is justified since we have to provide a mechanism for the Use Case base interface to accept as many parameters of any type as the implementations need. See the line:

dataverse-client-javascript/src/core/domain/useCases/UseCase.ts

Line 2 in ba18a0a

execute(...args: any[]): Promise<T>;

.

I couldn't find any other way to implement this, other than using "any". I'll be happy to refactor it if we find a better approach.

Thanks for noticing.

[pdurbin]

Should this be the h1? The feature request template has "overview" as the first line.

[GPortas]

I have decided to use ## (h2) in all section titles of the templates. So the titles are not too big. This is how I've often seen it in Dataverse issues.

[tainguyenbui]

loving these conversations ❤️, great time for me to learn too, thank you

[pdurbin]

Looking good now. Thanks, @GPortas! And thanks also to @MellyGray for suggesting ways to make the library better!

[pdurbin]

@GPortas I just pushed the tweak to the docs I mentioned: 8661503

Please take a look and revert or fix up as necessary! Thanks!

[kcondon]

Having trouble building locally:
npm run build

js-dataverse@1.0.48 build
tsc

sh: tsc: command not found

What am I doing wrong? Probably something basic.

[tainguyenbui]

Having trouble building locally: npm run build

js-dataverse@1.0.48 build
tsc

sh: tsc: command not found

What am I doing wrong? Probably something basic.

You may not have the typescript installed in your machine.

Here you can find info on how to do it https://www.npmjs.com/package/typescript

[pdurbin]

@kcondon I had the same problem: #55 (comment)

In the end the solution was npm install.

[kcondon]

To test this functionality we have to import the npm package from a test application. Usually we would install the package from the npm registry, but since it is a development unpublished version, we have to reference a pre-built local package from the test application.

To build the package locally:

• 1. Install dependencies omitting dev: npm install --omit=dev.
• 2. Build the package code: npm run build.
• 3. Create the .tgz local package: npm pack.

Hi, @tainguyenbui Installing typescript allowed me to build npm, thanks! I also needed to install jest to run the tests.

[kcondon]

@kcondon I had the same problem: #55 (comment)

In the end the solution was npm install.

What about the npm build instructions in the testing section then? Normally I would just run npm install to start but I was assuming that would fetch some preexisting version and the special build instructions would do something different or is that not the case?

[pdurbin]

I thought I clicked approve already. Whoops! Doing so now.