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

Introduce lucid-evolution and wasm compatibility to the frontend #64

Merged
merged 16 commits into from
Jan 15, 2025

Conversation

colll78
Copy link
Collaborator

@colll78 colll78 commented Jan 10, 2025

Add lucid-evolution and wasm support.

@KJES4 KJES4 self-requested a review January 14, 2025 15:28
@KJES4 KJES4 requested a review from j-mueller January 14, 2025 15:28
Copy link
Collaborator

@j-mueller j-mueller left a comment

Choose a reason for hiding this comment

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

Left some comments, also I'm not sure why CI doesn't work on this branch. Hopefully it will work when the merge conflicts have been resolved

import { Blockfrost, CML, Lucid, LucidEvolution, TxSigned, walletFromSeed } from "@lucid-evolution/lucid";

export async function makeLucid() {
const API_KEY = process.env.NEXT_PUBLIC_BLOCKFROST_API_KEY;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does this variable come from in the browser?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The api key is defined in an env.local file, each person will need to set this up on their own local instance or it will need to be defined in I believe the Docker file

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, does this mean we have to serve the page with next.js? So we can't have a static page with just HTML+JS+CSS?

Copy link
Collaborator

Choose a reason for hiding this comment

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

NextJS environment variables are injected at build time and I prefixed them with the NEXT_PUBLIC_ prefix so this would make them accessible on the client side. They will just become static after the build and if you want to switch them out you will need to rebuild everything.
Here's the docs that explain how this works: https://nextjs.org/docs/pages/building-your-application/configuring/environment-variables#loading-environment-variables



/** @type {import('next').NextConfig} */
const nextConfig = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need a line output = "export" for the docker image (to generate a single static page). This line won't work with the headers bit, so maybe we need two different configs, or look at debug vs production flags?

Copy link
Collaborator

@KJES4 KJES4 Jan 14, 2025

Choose a reason for hiding this comment

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

I can do that but then it will most likely create conflicts with the rewrites. This is what I get from NextJS when I try to use both the rewrites and the output="export" at the same time. https://nextjs.org/docs/messages/export-no-custom-routes

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the production build we don't need the rewrites anyway, so we can just switch between two different configs based on the flag

Copy link
Collaborator

@KJES4 KJES4 Jan 14, 2025

Choose a reason for hiding this comment

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

Should I comment out the rewrites in the commit so that the Nix build works?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be ok now

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the tests still failed. Should I merge it or wait for that to pass?

@j-mueller
Copy link
Collaborator

The nix build failed because of networking issues. I restarted it so hopefully it will pass now. Then we can merge this PR - it will break the static build for now, but I have some ideas for fixing it, which I'm going to try out tomorrow.



/** @type {import('next').NextConfig} */
const nextConfig = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be ok now

@KJES4 KJES4 merged commit 62a1d48 into main Jan 15, 2025
3 checks passed
@KJES4 KJES4 deleted the frontend-lucid branch January 15, 2025 12:20
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.

3 participants