feat(npm-aqua-compiler): create package #401

akim-bow · 2023-12-13T15:59:58Z

No description provided.

linear · 2023-12-13T16:00:01Z

DXJ-566 Add package to js client ecosystem which convert npm list format to aqua compiler format

packages/core/npm-aqua-compiler/package.json

packages/core/npm-aqua-compiler/src/imports.ts

shamsartem · 2023-12-13T16:40:10Z

packages/core/npm-aqua-compiler/src/imports.ts

+  return await gatherImportsFromArborist(arb);
+}
+
+export async function gatherImportsFromArborist(


what is the reason to have this as a separate function? do we plan on using it separately from the gatherImportsFromNpm function?

Agree. Probably we need a former function only

shamsartem · 2023-12-13T16:41:32Z

packages/core/npm-aqua-compiler/src/imports.ts

+  const arb = new Arborist({ path });
+
+  return await gatherImportsFromArborist(arb);


Suggested change

const arb = new Arborist({ path });

return await gatherImportsFromArborist(arb);

return gatherImportsFromArborist(new Arborist({ path }));

shamsartem · 2023-12-13T16:45:37Z

packages/core/npm-aqua-compiler/src/imports.ts

+
+      for (const edge of node.edgesOut.values()) {
+        // Skip dependencies that are not installed.
+        if (edge.to === null) {


for me it vscode says that edge.to is never null. Is it on practice sometimes null? If so - for me eslint has a warining here because of that

As I mentioned in slack, types for arborist are improper. While I was testing this code, I got null in edge.to. As far as I understand, this will be null for not installed deps (for some reason). @akim-bow please try to test this function on some big project with a lot of deps.

shamsartem · 2023-12-14T11:28:19Z

packages/core/aqua-to-js/src/common.ts

        item.tag === "labeledProduct" ? Object.values(item.fields) : item.items;

-      if (args.length === 1) {
+      if ("0" in args) {


these are actually different. Previously you checked the length is exactly equal to one. Now you are checking that first element exists. Does it make sense to still check the the length is exactly === 1. Can it ever be !== 1? If this is something aqua compiler enforces - I would probably check the case when it's not 1 and maybe throw error - this we will get a chance to discover some unexpected bug

the same question to think about is for the same check you are doing bellow

Wil recheck these in other files

shamsartem · 2023-12-14T11:40:42Z

packages/core/js-client/src/api.spec.ts

+      if (!("demoCalc" in functions)) {
+        throw new Error("Aqua code must contain demoCalc function");
+      }
+
      const { script } = functions["demoCalc"];

+      if (!("Calc" in typedServices)) {
+        throw new Error("Aqua code must contain Calc service");
+      }
+


both can be asserts, because it's inside test. You can leave it as it currently is as well

Would prefer to do assert in test

shamsartem · 2023-12-14T12:48:22Z

packages/core/js-client/src/compilerSupport/services.ts


      setTimeout(() => {
-        rejectCallback(err);
+        rejectCallback(err ?? "Error hasn't been provided");


I am currious when that can happen. I think would be a bit weird to see in the console "Error hasn't been provided"

This can happen if someone wrote incorrect air code and hasn't passed any args there.

Updated to a more lucky description

Still I think it's hard to understand for the user what is he supposed to do. If it's most likely a problem with the complier - then the error should say that as well

shamsartem · 2023-12-14T13:02:16Z

packages/core/js-client/src/api.ts

+
+      if (serviceImplValue === undefined) {
+        throw new Error(
+          `Expected to have ${schemaKey} in service implementation according to schema`,


Does this happen if user didn't implement the service he was supposed to implement? Or when does it happen?

I would prefer a more actionable error. Users wouldn't know what's the schema what's the key and where, especially if they don't use typescript

InversionSpaces · 2023-12-14T14:42:38Z

packages/core/js-client/src/api.ts

+    def.arrow.codomain.items.length === 1 &&
+    "0" in def.arrow.codomain.items
      ? def.arrow.codomain.items[0]
      : def.arrow.codomain;


Don't you want to extract this pattern to some function? It is repeated at least 3 times

probably possible with this

function isArrayWithOneElement<T>(arr: Array<T>): arr is [T] { return arr.length === 1 && arr[0] !== undefined; }

Will keep it in mind. This PR already bloated too much, i will do this in the next PR.

InversionSpaces · 2023-12-14T14:46:24Z

packages/core/npm-aqua-compiler/.gitignore

@@ -0,0 +1 @@
+package-lock.json


Why not storing package-lock.json in git?

Not sure should i keep it or no.
When this file is present and some dependencies changes e.g. someone adds more packages to tests, npm just breaks without meaningful error.

Without this file in git there is a less chance to fight with strange bugs when developing tests

From the other side this file will keep the tests more persistent, would be easier to track changes. Probably i will keep it, thanks.

InversionSpaces · 2023-12-14T14:49:13Z

packages/core/npm-aqua-compiler/src/imports.ts

+export async function gatherImportsFromNpm(
+  path: string,
+): Promise<Record<string, string[]>> {


This does not return imports according to the latest spec.
https://www.notion.so/fluencenetwork/Aqua-Imports-Spec-082d48d2d7184b05a3f47f35b781c843
Type of new imports is Record<string, Record<string, string[] | string>>. Read more in the doc.

Yeah I totally forgot we decided on a different structure. Thanks for the catch.

InversionSpaces · 2023-12-14T14:59:02Z

A vast part of changes are not related to DXJ-566 in any way as far as I understand. Why do like so...

shamsartem · 2023-12-14T15:09:39Z

A vast part of changes are not related to DXJ-566 in any way as far as I understand. Why do like so...

Yeah would probably better to do refactoring in a separate PR, would be easier to review. But I support this refactoring in generall, even though I agree this is probably not the best place to do that as part of this PR

shamsartem

nice. thanks

Akim Mamedov added 2 commits December 13, 2023 22:57

Add npm-aqua-compiler package

3422e2c

Release new package

29cd3aa

shamsartem reviewed Dec 13, 2023

View reviewed changes

packages/core/npm-aqua-compiler/package.json Outdated Show resolved Hide resolved

shamsartem reviewed Dec 13, 2023

View reviewed changes

packages/core/npm-aqua-compiler/src/imports.ts Outdated Show resolved Hide resolved

shamsartem reviewed Dec 13, 2023

View reviewed changes

Akim Mamedov added 2 commits December 14, 2023 16:47

Remove noUncheckedIndexedAccess from tsconfig.json

7643705

Fix a test script

f6f4bdc

shamsartem reviewed Dec 14, 2023

View reviewed changes

Akim Mamedov added 2 commits December 14, 2023 18:56

Fix length checks

5573fe4

Fix

4cacba6

shamsartem reviewed Dec 14, 2023

View reviewed changes

Update error description

afa55ad

shamsartem reviewed Dec 14, 2023

View reviewed changes

Try to choose a nicer err message

1585231

InversionSpaces reviewed Dec 14, 2023

View reviewed changes

Akim Mamedov added 5 commits December 15, 2023 00:06

New import format and API

2f838c7

Fix error message

bc34d4b

Improve test

6eb5dc8

Don't add empty string key when globalImports prop is empty

065c997

Fix exports

4ccdf70

shamsartem approved these changes Dec 15, 2023

View reviewed changes

fluencebot mentioned this pull request Dec 15, 2023

chore: release master #404

Merged

		const arb = new Arborist({ path });

		return await gatherImportsFromArborist(arb);

		@@ -0,0 +1 @@
		package-lock.json No newline at end of file

feat(npm-aqua-compiler): create package #401

feat(npm-aqua-compiler): create package #401

Uh oh!

Conversation

akim-bow commented Dec 13, 2023

Uh oh!

linear bot commented Dec 13, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akim-bow Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akim-bow Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

InversionSpaces Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

InversionSpaces commented Dec 14, 2023

Uh oh!

shamsartem commented Dec 14, 2023

Uh oh!

shamsartem left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

akim-bow Dec 14, 2023 •

edited

Loading

akim-bow Dec 14, 2023 •

edited

Loading

InversionSpaces Dec 14, 2023 •

edited

Loading