wasp icon indicating copy to clipboard operation
wasp copied to clipboard

User defined imports name mangling

Open infomiho opened this issue 3 months ago • 2 comments

I've had some issues with conflicting imports when integrating Arctic https://github.com/wasp-lang/wasp/pull/1851 and implemented this mangling to help.

Closes #1062 Closes #284

infomiho avatar Mar 07 '24 19:03 infomiho

Btw, your PR description (as its written now) won't trigger automatic closing of both issues. I got burned by this several times.

Instead of:

Closes #123 and #234

Do

Closes #123
Closes #234 

Context: https://github.com/nus-cs2103-AY2021S2/forum/issues/293

sodic avatar Mar 08 '24 15:03 sodic

Okay, so we need to unify the way we mangle the imports. I found the following using your regex:

  • sdk/wasp/server/operations/actions/index.ts and sdk/wasp/server/operations/queries/index.ts Found in OperationsGenerator -> ("_ext" suffix using applyExtImportAlias and extImportToJsImport 🟣)

  • sdk/wasp/server/webSocket/index.ts Found in WebSocketGenerator using the same fn as OperationsGenerator 🟣

  • server/src/routes/apis/index.ts Found in ApiRoutesG (custom "wasp_" prefix 🔵)

  • server/src/middleware/globalMiddleware.ts Found in ServerGenerator (custom "_waspGlobalMiddlewareConfigFn" 🔵)

  • server/src/jobs/*.ts Found in JobGenerator (custom "_waspJobDefinition" 🔵)

🔵 manual alias 🟣 using the applyExtImportAlias and extImportToJsImport

Okay, that seems manageable. Great call to ripgrep!

infomiho avatar Mar 08 '24 16:03 infomiho

The biggest issue I'd like to resolve to make this "proper name mangling" is to somehow keep track of aliases that were used and ensure no duplicate aliases are used in the same file (thus breaking our code!).

Two cases

Happy path (works now)

Let simplest thing we can have in a file:

  • import { foo as foo__userDefined } from ...
  • import { bar as bar__userDefined from ...

This is all fine and in the basic case of nothing goes wrong since there are no duplicate import names i.e. we have foo and bar here.

Duplicate names

What if we had a situation like this in a single template file:

  • import { foo as foo__userDefined } from ...
  • import { foo as foo__userDefined } from ...

We now have duplicate names! foo and foo are both in the same file. This gives us foo__userDefined and foo__userDefined aliases. This would cause our code to break ⚠️

Solutions

Ideally, we would do it like e.g. Rollup does it and keep a global store of aliases and then on duplicate, we would just add a number at the end.

Ideal solution (keep track at template level)

If we kept the imports unique per-file, it might look like this:

File1:

  • import { foo as foo__userDefined$0 } from ...
  • import { foo as foo__userDefined$1 } from ...

File2:

  • import { foo as foo__userDefined$0 } from ...
  • import { foo as foo__userDefined$1 } from ...

Okay solution IMHO (keep track at global level)

Same like the previous solution, but we could have a "global state" and no per-file scoping:

File1:

  • import { foo as foo__userDefined$0 } from ...
  • import { foo as foo__userDefined$1 } from ...

File2:

  • import { foo as foo__userDefined$2 } from ...
  • import { foo as foo__userDefined$3 } from ...

infomiho avatar Mar 11 '24 19:03 infomiho

All of the code that needed name mangling needed to come inside of the Generator monad, which wasn't too much of a difference since we used the JsImport in FileDrafts anyways.

infomiho avatar Mar 25 '24 14:03 infomiho

@infomiho I am curious, when does this situation where two user-defined symbols come into conflict happen? Does it happen if they have a query and action with the same name, or something like that? I am asking, because I wonder if we are even ok with allowing them to do something like that. But I am guessing there is some other use case you were considering here.

Because for me, the main thing I wanted us to fix with name mangling was ensuring the user-defined symbols don't come into conflict with the symbols that we defined.

Martinsos avatar Mar 26 '24 00:03 Martinsos