Skip to content

Feature/prd 1116 - #662

Open
cristian-situm wants to merge 16 commits into
developfrom
feature/PRD-1116
Open

Feature/prd 1116#662
cristian-situm wants to merge 16 commits into
developfrom
feature/PRD-1116

Conversation

@cristian-situm

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread .yarnrc.yml
Comment thread plugin/package.json
],
"devDependencies": {
"@babel/core": "^7.22.5",
"@babel/eslint-parser": "^7.22.5",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Porfa revisade que ningun paquete de desarrollo e necesario para desarrollar o plugin. Pedinlle a OpenCode que revisara todas as dependencias que non se estaban a usar no proyecto e quitou todas estas; reviseinas e non boto ningunha en falta

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non entendo que é o que nos pides. Penso que esa revisión é parte da tarefa, non?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Simplemente un repaso visual de que non estou eliminando ninguna dependencia que sexa importante

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

O analisis das dependencias que eliminei deixeino no ticket

Comment thread plugin/NOTICE.md
@@ -0,0 +1,67 @@
# Notices for @situm/react-native

@cristian-situm cristian-situm Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Este texto legal deberia de revisalo Canedo / Angel / Cris / alguen fora de desarrollo ?

Preguneille a OpenCode e en teoria non e un texto legal ou xuridico. Simplemente e un texto para cumplir cas indicacions das licencias das nosas dependencias

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Simplemente revisa o formato do SDK de Android e tira.

Comment thread plugin/NOTICE.md Outdated
@@ -0,0 +1,67 @@
# Notices for @situm/react-native

Copyright (c) 2023 Situm Technologies

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2023?

@cristian-situm cristian-situm Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Puxome co Copyright co ano 2023 porque e o ano que ven definido no LICENSE do repo. Entendo que e correcto non ? Ou ao crear este archivo este ano deberia de por 2026 ?

@cristian-situm cristian-situm Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Vale revisado; polo visto no SDK usamos un rango de anos e esta forma e a habitual -> (ano que se creou o plugin) - (ano actual)

Comment thread plugin/NOTICE.md
@@ -0,0 +1,67 @@
# Notices for @situm/react-native

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Simplemente revisa o formato do SDK de Android e tira.

Comment thread plugin/NOTICE.md Outdated
Comment thread plugin/package.json
],
"devDependencies": {
"@babel/core": "^7.22.5",
"@babel/eslint-parser": "^7.22.5",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Non entendo que é o que nos pides. Penso que esa revisión é parte da tarefa, non?

Comment thread .yarnrc.yml
Comment thread package.json
@@ -7,7 +7,12 @@
},
"resolutions": {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Que é esto?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

E un apartado para fijar versions de dependencias transitivas; para que cando yarn instale, por ejemplo, react-native, e meta as suas propias dependencias (p.e. brace-expansion) pois que instale unha versions sin vulnerabilidades

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Esto entón paréceme peligroso... Si o integrador pode sobrescribir as versións, por que non deixalo da súa man?
Reconozco que no SDK de android estamos facendo o mismo con OkIo, pero fixéronse probas concretas para ver que non rompíamos nada a nivel comunicationManager e ademais o integrador sigue podendo escoller. React-native paréceme moito máis complexo en canto a xestión de paquetes, dame algo de miedito.
Que ocorre si a instalación do integrador quería resolver unha versión maior e nós estamos levándoo a unha menor? Meter este cambio vainos obligar a estar constantemente revisando? Ademais parece que estás pineando versións... Si queres declarar mínimos aínda o vexo, pero así cóstame.
Ou quizais non estou entendendo ben como funciona ollo.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ollo, estas resolucions son a nivel de repo porque estou declarando este "resolutions" no package.json raiz.
Estas versions pinneadas solo aplican para a xente que use o repo directamente, xa que distribuimos por npm unicamente o plugin/package.json

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Entón que utilidade ten o cambio?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Para que a nivel de repo non poidamos meter vulnerabilidades. E dicir, sin estas "resolutions" podriamos estar metendo paquetes comprometidos a hora de desarrollar

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok, eu non o teño claro pero si ti o tes claro adiante.

Comment thread plugin/package.json
Comment thread azure-pipelines.yml Outdated
Comment thread plugin/scripts/verify-licenses.sh Outdated
@@ -0,0 +1,219 @@
#!/usr/bin/env bash

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Este archivo resulta na seguinte execucion

Image

Comment thread plugin/scripts/check-npm-notice.js
Comment thread plugin/scripts/prepack.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

E non era máis fácil simplemente facer un cp?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Era mais facil, pero si o dia de manhan rompe o CI e temos que facer a release manual estariamos sacando releases mal feitas.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Digo facer "cp" no package.json en vez de "node scripts/prepack.js", paréceme que copiar dous ficheiros usando js é moi esaxerado cando existe un comando cp que xa o fai.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Pois tes toda a razon do mundo. Vou darlle unha ultima volta para simplificar o pre empaquetado e de paso tamen o bash de verificacion de estado legal

Comment thread plugin/scripts/verify-licenses.sh Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

O output está chulísimo, pero o contido do script é moi duro... Por min adiante, pero manter esto será cousa de IAs, non nosa.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sep, correcto. No caso de querer metelo a futuro nun CI e formalizalo penso que convendria pedirlle a unha IA que pense noutra forma de facer este script, nun so lenguaje sin mezclar con node e reducindo a logica

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yo lo único que puedo ver mal aquí es que si el script falla o no hace lo que pensamos no lo vamos a saber por la complejidad. Si asumimos que funciona y no es verdad nos vamos a comer el error

…on + turn verify-licenses.sh to a .js file to simplify complex bash script
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants