User natives #206

lgassman · 2024-12-30T00:26:13Z

Este PR necesita de que se aprueba primero uqbar-project/wollok-ts#326

Agrega capacidad para que un proyecto wollok pueda incluir métodos nativos.

Además de los tests que se incluyen, se puede ver los siguientes proyectos que utilizan esta feature:

dolarito: https://github.com/wollok/dolarito : Utiliza métodos nativos para acceder a internet
pelotas: https://github.com/wollok/pelotas : Utiliza métodos nativos para que a través de una librería de node, se persista información.
pruebaNatives: https://github.com/wollok/pruebaNatives : Esta es para tener una referencia rápida de como escribir un método nativo complejo (pasandose parámetros, haciendo invocaciones, etc).

Los archivos TS/JS se pueden poner al mismo nivel que el .wlk que los utiliza, o bien en una carpeta interna. ésta se puede elegir en el init del proyecto y queda almacenada en package.json.

Incluye un nuevo comando dependencies, con tres subcomandos: add, remove, sync, que funcionan como un wrapper de npm. Eso quizás requiera discusión, porque entiendo que podríamos usar directamente npm sobre el proyecto y tiene que andar, pero me pareció más piola ocultar la complejidad de npm.

Las librerías quedan en node_modules a nivel proyecto. Está carpeta ahora se incliuye en el .gitIgnore.

También se resuelve un bug existente en el "assert path exist" y se agrega capacidad de checkeo en json.

Se agrega la abtracción Project encargado de interactuar con el package.json, y capaz de leer los métodos nativos.

Como trabajo futuro me gustaría discutir de que manera conviene incluir acceso a las firmas de los métodos de runtimeModel para que las funciones nativas sean tipadas como corresponde

lgassman · 2024-12-30T00:41:31Z

package-lock.json

        "winston": "^3.11.0",
-        "wollok-ts": "^4.1.9",
+        "wollok-ts": "file:../wollok-ts",


Ojo acá! me quedó porque en mi compu laburé siempre contra mi propia version de wollok-ts, pero para pushear volví a poner la versión releaseada. No se bien como manejan el proceso de release cuando hay interdependencias entre los proyectos

lgassman · 2024-12-30T00:45:28Z

src/commands/repl.ts

@@ -108,6 +108,7 @@ export function interpreteLine(interpreter: Interpreter, line: string): string {
 export async function initializeInterpreter(autoImportPath: string | undefined, { project, skipValidations }: Options): Promise<Interpreter> {
  let environment: Environment
  const timeMeasurer = new TimeMeasurer()
+  const proj = new Project(project)


paso por Project para leer las natives

Hay alguna razón por la que decidiste crear un objeto? Por lo que veo solamente se instancia para mandarle readNatives() en todos los usos.

lgassman · 2024-12-30T00:46:03Z

src/commands/init.ts

  const project = join(_project, folder ?? '')
+  const nativesFolder = join(project, natives ?? '')


si me pasaron natives, entonces es esa carpeta, si no es la misma del project

lgassman · 2024-12-30T00:58:28Z

src/utils.ts

+    module: 'NodeNext',
+    moduleResolution: 'NodeNext',
+  },
+})


De estas opciones no estoy seguro si son las mejores. Entiendo que esto es necesario para cargar los archivos ts dinámicamente.

Uh nunca use asi ts-node asi que no tengo ninguna opinion fuerte, cli se traspila a commonjs no se si eso traeria problemas.

Lo que si me pregunto es si esta bien que este como dev dependency en el package.json

lgassman · 2024-12-30T01:00:07Z

src/utils.ts

+    } catch (_error) {
+      // No package.json or it is invalid. This is not a real problem.
+      // Silence here or log?
+    }


Esto me da mucha duda. Llegué a la conclusión que lo mejor es que si no pudo cargar el package.json hay que hacerse el gil. Pero no se que opinan ustedes

Un log seguro tiraria, cortar la ejecución para mi no.

+1 al warning.

Pero atención que acá se repite lógica con readPackageProperties (que se usa para leer la carpeta assets). Estaría bueno unificar y loggear lo correspondiente en ambos casos: ya sea sugerir que se cree el package.json (no tenemos forma de hacer eso con el comando init no?) o que el archivo está roto.

lgassman · 2024-12-30T01:06:20Z

test/assertions.ts

-      pathExists(path: string): Assertion
+      pathExists: Assertion
+      jsonKeys(expectedKeys: string[]): Assertion;
+      jsonMatch(expected: Record<string, any>): Assertion;


Agregué para hacer assertions sobre el json, pero también arreglé el pathExist que no estaba bien

Piola! Acabo de mergear #205 que arreglaba lo del pathExist y lo teníamos colgado hace tiempo. Perdón por el conflicto, pero no quería más gente arreglando eso mientras esto se mergea 😅

ivojawer

Wooo cuanta magia que hay aca 🪄

Algo medio sad es que las natives no van a andar para el debugger 😢. Hay que crear ese issue en el LSP para no olvidarse de algun dia implementarlo

ivojawer · 2025-01-05T16:50:45Z

src/utils.ts

+    } catch (_error) {
+      // No package.json or it is invalid. This is not a real problem.
+      // Silence here or log?
+    }


Un log seguro tiraria, cortar la ejecución para mi no.

ivojawer · 2025-01-05T16:53:14Z

src/utils.ts

+    module: 'NodeNext',
+    moduleResolution: 'NodeNext',
+  },
+})


Uh nunca use asi ts-node asi que no tengo ninguna opinion fuerte, cli se traspila a commonjs no se si eso traeria problemas.

Lo que si me pregunto es si esta bien que este como dev dependency en el package.json

src/index.ts

PalumboN

GROSIIISIMO! 🚀 👨‍🎤 💯 🈴

Dejé un par de comentarios para ir charlando!

Como dijo Ivo, hay que ver cómo se lleva todo esto con el empaquetado (habría que generarlo y probar el ejemplo contra eso).

PalumboN · 2025-01-04T18:59:05Z

src/commands/repl.ts

@@ -108,6 +108,7 @@ export function interpreteLine(interpreter: Interpreter, line: string): string {
 export async function initializeInterpreter(autoImportPath: string | undefined, { project, skipValidations }: Options): Promise<Interpreter> {
  let environment: Environment
  const timeMeasurer = new TimeMeasurer()
+  const proj = new Project(project)


Hay alguna razón por la que decidiste crear un objeto? Por lo que veo solamente se instancia para mandarle readNatives() en todos los usos.

PalumboN · 2025-01-04T19:03:17Z

src/commands/repl.ts

@@ -22,6 +21,7 @@ export type Options = {
  host: string,
  port: string,
  skipDiagram: boolean,
+  natives?: string


Esto no iría acá, no? Es un parámetro del init solamente

PalumboN · 2025-01-04T20:40:08Z

src/commands/dependencies.ts

+export const addDependency = (pkg: string, { project, verbose }: Options): void => {
+  executeNpmCommand(`install ${pkg}`, project, verbose)
+}
+
+export const removeDependency = (pkg: string, { project, verbose }: Options): void => {
+  executeNpmCommand(`uninstall ${pkg}`, project, verbose)
+}


A estos no hay que mandarles -s para que queden guardados en el package.json?

src/index.ts

PalumboN · 2025-01-05T17:47:40Z

src/index.ts

@@ -64,8 +64,38 @@ updateNotifier().finally(() => {
    .option('-t, --noTest', 'avoids creating a test file', false)
    .option('-c, --noCI', 'avoids creating a file for CI', false)
    .option('-ng, --noGit', 'avoids initializing a git repository', false)
+    .option('-N, --natives [natives]', 'Folder name for native files and dependencies (defaults to root project).', undefined)


Pequeño cambio para mantener el TOC con el resto de las options

Suggested change

.option('-N, --natives [natives]', 'Folder name for native files and dependencies (defaults to root project).', undefined)

.option('-N, --natives [natives]', 'folder name for native files (default: root folder).', undefined)

Entiendo que las dependencias siempre se instalan en node_modules.

PalumboN · 2025-01-05T17:51:50Z

src/index.ts

+  const dependencyCommand = new Command('dependencies')
+    .description('Manage dependencies for a Wollok project')


Dos cositas:

Por qué hacer un wrapper de npm y no usar npm directo? Entiendo que en las máquinas de los labos donde la instalación es por el compilado pueden no tener npm, pero en ese caso este comando no va a andar tampoco. En el caso recomendado los usuarios ya usan npm para instalar wollok.

Esto hay que documentarlo en algún lado (por ahora lo mejor es el README, pero necesitamos una wiki/doc con ejemplos de los comandos (para otro issue).

PalumboN · 2025-01-05T17:55:18Z

src/utils.ts

+    } catch (_error) {
+      // No package.json or it is invalid. This is not a real problem.
+      // Silence here or log?
+    }


+1 al warning.

Pero atención que acá se repite lógica con readPackageProperties (que se usa para leer la carpeta assets). Estaría bueno unificar y loggear lo correspondiente en ambos casos: ya sea sugerir que se cree el package.json (no tenemos forma de hacer eso con el comando init no?) o que el archivo está roto.

PalumboN · 2025-01-05T18:03:34Z

test/assertions.ts

-      pathExists(path: string): Assertion
+      pathExists: Assertion
+      jsonKeys(expectedKeys: string[]): Assertion;
+      jsonMatch(expected: Record<string, any>): Assertion;


Piola! Acabo de mergear #205 que arreglaba lo del pathExist y lo teníamos colgado hace tiempo. Perdón por el conflicto, pero no quería más gente arreglando eso mientras esto se mergea 😅

PalumboN · 2025-01-05T18:11:45Z

test/init.test.ts

+
+    expect(join(project, 'myNatives')).to.pathExists
+    expect('package.json')
+    expect(join(project, 'package.json')).jsonMatch({ natives: 'myNatives' })


Acá no quedaría mejor usar la función que trae el JSON desde el archivo que se usa en los otros comandos?

Además no se puede usar .property[Val] para las assertions?

PalumboN · 2025-01-05T18:13:16Z

test/assertions.ts

+  const getNestedValue = (obj: Record<string, any>, path: string): any =>
+    path.split('.').reduce((acc, key) => acc ? acc[key] : undefined, obj)
+
+  const matchPartial = (
+    expected: Record<string, any>,
+    actual: Record<string, any>,
+    prefix: string = ''
+  ): boolean =>
+    Object.keys(expected).every((key) => {
+      const fullPath = prefix ? `${prefix}.${key}` : key
+      if (typeof expected[key] === 'object' && expected[key] !== null) {
+        if (typeof actual[key] !== 'object' || actual[key] === null) {
+          return false
+        }
+        return matchPartial(expected[key], actual[key], fullPath)
+      }
+      return actual[key] === expected[key]
+    })
+
+  const getJsonContent = (filePath: string): any => {
+    if (!existsSync(filePath)) {
+      throw new chai.AssertionError(`Expected file "${filePath}" to exist`)
+    }
+
+    try {
+      return JSON.parse(readFileSync(filePath, 'utf8'))
+    } catch (error) {
+      throw new chai.AssertionError(
+        `Failed to parse JSON from file "${filePath}": ${String(error)}`
+      )
+    }
+  }


Hace falta todo esto? La API de chai es bastante completa.

lgassman added 22 commits December 12, 2024 16:50

add test to work

a6dd72c

Removed submodule reference

45ffb98

Removed submodule reference

5de08e5

partial commit

d82c6ff

read natives in tests

973aeb4

Options refactor

d1ffb70

fix path assertions

7d0a59d

set folder as part of options

55106f0

fix init call

b3dcb4b

add natives to package.json

e3ffc1e

add Options class to REPL

2f9c7ad

use base options on repl

429df79

run options

f6fb2b2

add natives options to init

2cb3d02

fix run

b213aa8

use typescript in runtime

2529174

revert BaseOptions to use type

d611fd8

read natives project inside

e707152

dependency command

79c9622

dolarito

bb67551

change dependecy command name and add log to .gitignore

fe89030

restore wollok-ts dependency

0d7eb58

lgassman requested review from fdodino and PalumboN December 30, 2024 00:27

restore package.json

6f1dd1b

lgassman commented Dec 30, 2024

View reviewed changes

remove unused features of project

d81b75e

lgassman commented Dec 30, 2024

View reviewed changes

lgassman requested a review from ivojawer December 30, 2024 01:12

ivojawer reviewed Jan 5, 2025

View reviewed changes

PalumboN approved these changes Jan 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User natives #206

User natives #206

lgassman commented Dec 30, 2024 •

edited

Loading

lgassman Dec 30, 2024

lgassman Dec 30, 2024

PalumboN Jan 4, 2025

lgassman Dec 30, 2024

lgassman Dec 30, 2024

ivojawer Jan 5, 2025

lgassman Dec 30, 2024

ivojawer Jan 5, 2025

PalumboN Jan 5, 2025

lgassman Dec 30, 2024

PalumboN Jan 5, 2025 •

edited

Loading

ivojawer left a comment

ivojawer Jan 5, 2025

ivojawer Jan 5, 2025

PalumboN left a comment

PalumboN Jan 4, 2025

PalumboN Jan 4, 2025

PalumboN Jan 4, 2025

PalumboN Jan 5, 2025

PalumboN Jan 5, 2025

PalumboN Jan 5, 2025

PalumboN Jan 5, 2025 •

edited

Loading

PalumboN Jan 5, 2025

PalumboN Jan 5, 2025

		const project = join(_project, folder ?? '')
		const nativesFolder = join(project, natives ?? '')

	.option('-N, --natives [natives]', 'Folder name for native files and dependencies (defaults to root project).', undefined)
	.option('-N, --natives [natives]', 'folder name for native files (default: root folder).', undefined)

		const dependencyCommand = new Command('dependencies')
		.description('Manage dependencies for a Wollok project')

User natives #206

Are you sure you want to change the base?

User natives #206

Conversation

lgassman commented Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PalumboN Jan 5, 2025 • edited Loading

Choose a reason for hiding this comment

ivojawer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PalumboN left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PalumboN Jan 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgassman commented Dec 30, 2024 •

edited

Loading

PalumboN Jan 5, 2025 •

edited

Loading

PalumboN Jan 5, 2025 •

edited

Loading