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

Build error: Message SKD uses node specific import #4

Open
NilsJacobsen opened this issue Mar 19, 2024 — with Linear · 15 comments
Open

Build error: Message SKD uses node specific import #4

NilsJacobsen opened this issue Mar 19, 2024 — with Linear · 15 comments

Comments

Copy link
Member

When types get imported via @inlang/sdk in the settings component, it can't get build anymore, because it's not build for "--platform=node".

Error: R] Could not resolve "node:crypto"

    ../../../lix/source-code/client/dist/hash.js:6:34:
      6 │         usedCrypto = await import("node:crypto");
        ╵                                   ~~~~~~~~~~~~~

The package "node:crypto" wasn't found on the file system but is built into node. Are you trying to bundle for node? You can use "--platform=node" to do that, which will remove this error.

The problem origined on the lix side: with the hash.js file.

Copy link

linear bot commented Mar 19, 2024

Copy link
Member

@nils.jacobsen this is a lix issue no?

Copy link
Member

move this issue to lix-sdk?

@samuelstroschein samuelstroschein transferred this issue from opral/inlang-message-sdk Mar 19, 2024
Copy link

linear bot commented Mar 19, 2024

@samuelstroschein samuelstroschein closed this as not planned Won't fix, can't repro, duplicate, stale Mar 19, 2024
Copy link
Member

The typescript compiler likely detects the node import and throws.

A quick-fix could be to trick the typescript compiler. @jan.johannes @nils.jacobsen can you try this fix below in the lix sdk code?

-await import("node:crypto")
+await import("no" + "de" + ":crypto")

Copy link

resolved by using esbuild --external configuration. cause was building for browser with esbuild but esbuild had no way to know the dynamic import will never be active in browser environment

Copy link
Member Author

Thanks @jan.johannes gonna fix it.

Copy link
Member

samuelstroschein commented Mar 20, 2024

@jan.johannes why not use the hack i proposed?

it seems like your --external proposal now requires every lix app do know about this and add it to their build step? my proposal likely avoids that every lix app needs to build with --external. the problem is scoped to the lix sdk, not leaked to lix apps

@janfjohannes
Copy link

janfjohannes commented Mar 20, 2024

@samuelstroschein the hack does not work and this is the official esbuild solution. the only reason this is a problem is because tree shaking is still broken in our build, by the time we let lix be used by other apps this needs to be solved anyways.

Copy link

@nils.jacobsen @samuel.stroschein actually i just found a much better solution. the --external will be obsolete after the next lix release node exposes the module as globalThis.crypto too

Copy link
Member

@jan.johannes sounds like a one line code change. why not do it now?

@janfjohannes
Copy link

janfjohannes commented Mar 20, 2024

@samuelstroschein because it breaks tests for some reason, i need to find out why it works everywhere except in github actions :D

Copy link

janfjohannes commented Mar 21, 2024

ok so the globalThis was introduced in node 12 but only in node 19 do we get the crypto there. so for node 18 it only works with a command line flag. i moved to using the old method. but added a string interpolation into the dynamic import to try pushing esbuild into treating it as external. (i think thats what you meant by "no" + "de", but that does not work, ``node:${variable} should do the trick)

Copy link
Member

>(i think thats what you meant by "no" + "de", but that does not work, ``node:${variable} should do the trick)

Yes that's what i meant. I thought interpolation wouldn't be needed to achieve this but had in mind

Copy link

@samuel.stroschein anything should work as long as there is a variable added to a string in the process

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

No branches or pull requests

3 participants