regulaforensics / FaceSDK-web-js-client

Regula Face SDK web API js client for the browser and node.js based on axios
https://regulaforensics.com
5 stars 3 forks source link

Fix/cjs import base64 arraybuffer #22

Closed oxidia closed 2 years ago

oxidia commented 2 years ago

issue link

oxidia commented 2 years ago

@hleb-albau can you please have a look at this?

hleb-albau commented 2 years ago

@ivan-gil @ikliashchou could you please assist here, js is not my main language

@oxidia sorry for late reply, in review

oxidia commented 2 years ago

@hleb-albau Thank you 🙏

oxidia commented 2 years ago

Hello @ivan-gil, thank you for your response I am not so familiar with GitHub actions, my suggestion is to keep it simple, and just test the esm part of the examples! Can you please confirm? so my fix is as follow

name: run smoke test

on:
  push:
    branches:
      - master
  pull_request:
    branches:
      - master

jobs:
  run_smoke_test:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - uses: actions/setup-node@v1
        with:
          node-version: 14
          registry-url: https://registry.npmjs.org/
      - run: npm install
      - run: npm run build
      - run: npm install
-       working-directory: ./example
+       working-directory: ./example/esm
      - run: node --unhandled-rejections=strict index.js
-       working-directory: ./example
+       working-directory: ./example/esm
        env:
          API_BASE_PATH: "https://test-faceapi.regulaforensics.com"
ivan-gil commented 2 years ago
ivan-gil commented 2 years ago

Hello @ivan-gil, thank you for your response I am not so familiar with GitHub actions, my suggestion is to keep it simple, and just test the esm part of the examples! Can you please confirm? so my fix is as follow

name: run smoke test

on:
  push:
    branches:
      - master
  pull_request:
    branches:
      - master

jobs:
  run_smoke_test:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - uses: actions/setup-node@v1
        with:
          node-version: 14
          registry-url: https://registry.npmjs.org/
      - run: npm install
      - run: npm run build
      - run: npm install
-       working-directory: ./example
+       working-directory: ./example/esm
      - run: node --unhandled-rejections=strict index.js
-       working-directory: ./example
+       working-directory: ./example/esm
        env:
          API_BASE_PATH: "https://test-faceapi.regulaforensics.com"

Before you changes tests were run upon both esm and cjs environments as there was one index js. So for github action we should run test twice both for esm and cjs types of modules. I will extends PR soon and we can merge it

oxidia commented 2 years ago

Hello @ivan-gil, thank you for your response I am not so familiar with GitHub actions, my suggestion is to keep it simple, and just test the esm part of the examples! Can you please confirm? so my fix is as follow

name: run smoke test

on:
  push:
    branches:
      - master
  pull_request:
    branches:
      - master

jobs:
  run_smoke_test:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - uses: actions/setup-node@v1
        with:
          node-version: 14
          registry-url: https://registry.npmjs.org/
      - run: npm install
      - run: npm run build
      - run: npm install
-       working-directory: ./example
+       working-directory: ./example/esm
      - run: node --unhandled-rejections=strict index.js
-       working-directory: ./example
+       working-directory: ./example/esm
        env:
          API_BASE_PATH: "https://test-faceapi.regulaforensics.com"

Before you changes tests were run upon both esm and cjs environments as there was one index js. So for github action we should run test twice both for esm and cjs types of modules. I will extends PR soon and we can merge it

I didn't quite understand what should I do next, can you please put more details on this.