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

Init storage shipper #2

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

piotrooo
Copy link
Member

No description provided.

@piotrooo piotrooo self-assigned this Aug 10, 2023
@piotrooo piotrooo marked this pull request as ready for review August 28, 2023 07:20
Copy link

@BartPlaza BartPlaza left a comment

Choose a reason for hiding this comment

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

Sprawdziłem pod tym kątem co prosiłeś narazie bez zagłębiania się narazie w to co robią poszczególne funkcje. Ogólnie wygląda ok, kilka drobnych uwaga dodałem

@@ -0,0 +1,17 @@
export interface Artifact {

Choose a reason for hiding this comment

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

jeśli chodzi o typy to zazwyczaj robimy tak że:

  1. Jak projekt jest mały to w src/ jest jeden plik types.ts i tam są wszystkie typy/interfejsy
  2. Jak projekt jest większy to w scr/ robimy katalog types/ i tam wrzucamy osobne pliki tak jak Ty zrobiłeś

data: ArrayBuffer | null
}

export function sanitizedParentDir (artifact: Artifact): string | null | undefined {

Choose a reason for hiding this comment

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

tą funkcje proponuje przenieść do src/utils/artifact.ts

export * from './StorageShipper'
export * from './StorageUploader'

export * from './artifact/Artifact'

Choose a reason for hiding this comment

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

w tym pliku powinny być eksporty tylko tych funkcji, które użytkownik bibloteki może używać, bez eksportu typów (typy powinny sie przy buildzie wygenerować automatycznie jako pliki xyz.d.ts

export interface Deploy {
name: string
include: string
headers?: object

Choose a reason for hiding this comment

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

jak chcesz typ dla dowolnego objektu to lepiej użyć Record<string, unknown> (object to może być wszystko prawie, nawet null)

@@ -0,0 +1,63 @@
import type JSZip from 'jszip'
import { minimatch } from 'minimatch/dist/mjs'

Choose a reason for hiding this comment

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

racze sporadycznie jest potrzeba importowania z dist/, jeżeli zrobiłeś tak bo ts rzucał błędy to raczej kwestia braku typów, wtedy jesli biblioteka nie jest w TS to warto poszukać typów do niej -> https://www.npmjs.com/package/@types/minimatch

@@ -0,0 +1,23 @@
import * as schema from '../../../../schema/manifest-1.0.json'
import Ajv2020 from 'ajv/dist/2020'

Choose a reason for hiding this comment

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

tu podobnie do sprwadzenia czy są typy dostępne

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

Successfully merging this pull request may close these issues.

2 participants