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

In .args.mjs allow people to pass JS variables instead of strings #116

Closed
Pabl0cks opened this issue Sep 12, 2024 · 10 comments
Closed

In .args.mjs allow people to pass JS variables instead of strings #116

Pabl0cks opened this issue Sep 12, 2024 · 10 comments
Assignees
Labels
discussion Further information is discussed

Comments

@Pabl0cks
Copy link
Collaborator

No description provided.

@Pabl0cks Pabl0cks added the discussion Further information is discussed label Sep 12, 2024
@rin-st
Copy link
Member

rin-st commented Sep 12, 2024

Could you explain this? It's already allowed, you just need to add logic to template to handle that vars

@carletex
Copy link
Member

@rin-st I think it's related to this: #109 (comment)

@rin-st rin-st self-assigned this Sep 12, 2024
@rin-st
Copy link
Member

rin-st commented Sep 12, 2024

I tried to experiment with this and created draft pr #118 . Main idea is stringifying passed variables if it's not a string #118 (comment)

But solution has a (big) cons. Let's take a look at these extension variables: https://github.com/carletex/create-eth-extensions/blob/tailwind-extend-solidity-version/extension/packages/nextjs/tailwind.config.js.args.mjs

We need to be able to pass an object and string representation of lightTheme (same for darkTheme). These variables works fine by default

const lightTheme = {
  something: "#000"
  ...
}

and it's string representation like it worked before

const lightTheme = `{
  something: "#000"
  ...
}`

which used this way

themes: [
{
light: ${lightTheme},
},


[Issue 1]
Also we need to be able to pass an object

const extendTheme = {
      backgroundImage: {
        "gradient-radial": "radial-gradient(var(--tw-gradient-stops))",
      },
      colors: {
        "custom-blue": "#1E40AF",
      }
};

and it's string representation. Note that there's no "{" and "}" at the start and at the end

const extendTheme = `
      backgroundImage: {
        "gradient-radial": "radial-gradient(var(--tw-gradient-stops))",
      },
      colors: {
        "custom-blue": "#1E40AF",
      }
`;

which is used here:

extend: {
${extendTheme[0] && `${extendTheme[0]},`}
boxShadow: {

we meet first issue: we can't just pass an object here, so we need additional logic to remove that "{" and "}" if object is passed
https://github.com/scaffold-eth/create-eth/pull/118/files#r1757585413


[issue 2]
Consider this variable.

export const menuObjects = `{
  label: "Greetings",
  href: "/greetings",
  icon: <MagnifyingGlassIcon className="h-4 w-4" />,
}`;

if we want to pass an object, we can't pass it just like that, sending JSX here is a bad idea.

export const menuObjects = {
  label: "Greetings",
  href: "/greetings",
  icon: <MagnifyingGlassIcon className="h-4 w-4" />,
};

So we need to pass JSX as a string

export const menuObjects = {
  label: "Greetings",
  href: "/greetings",
  icon: '<MagnifyingGlassIcon className="h-4 w-4" />',
};

and again, we need additional dances to convert it to a valid result string, which is the same as first block on this [issue 2] https://github.com/scaffold-eth/create-eth/pull/118/files#r1757596314 . Works same way for any variables instead of JSX


[issue 3]
It could be dangerous if allow users to send an object sometimes, for example this (private keys could be passed as strings and sent to result template)


[issue 4]
everything will be stringified. So we need to be cautious about every variable. For example if we're passing undefined to some someVar, we need to understand that on template we will get someVar = "undefined" and we need to handle that cases


Probably there is a better way to pass an objects, but I didn't found it for now

@rin-st
Copy link
Member

rin-st commented Sep 12, 2024

Also, I'm thinking about aggregating strategies for every variable, because we may need different behaviors #109 (comment). Possible strategies: collect, single, merge.
Collect (act as current version): Take every variable value from args files and collect to array chainNames=[first, second, ...];
Single: take just one variable (first?) and return without array;
Merge: if we will use objects, we may need to return combined object {...objFromFirstArgFile, ...objFromSecondArgFile}

we can add this strategies config to withDefaults function as additional strategies param or something like that

export const withDefaults =

cc @technophile-04 to see this thread

@technophile-04
Copy link
Collaborator

Thanks @rin-st for nice research and tinkering and also other for bringing up this discussion!

After thinking and doing research for a while, I feel like we don't need to change anything at processTemplateFiles function and massage data inside .template.mjs file depending on variable. Because feels like making processTemplateFiles a way generic seem hard and need to consider lots of cases.

We tell people to pass objects where syntax wise its intuitive(tailwind conf, menuObjects, etc..) and strings for example passing code snippets (example in layout.template.mjs)

And we handle the logic at .template.mjs level it should be easy since we know what data will be coming. So an example to handle Issue 1 will be:

`tailwind.config.js.template.mjs` :
const contents = ({ lightTheme, darkTheme, extendTheme }) => {
  const massagedExtendedTheme = JSON.stringify(extendTheme[0]);

  return `  
  {
  ...
  theme: {
    extend: {
      ${extendTheme[0] && `...${massagedExtendedTheme},`}
      boxShadow: {
        center: "0 0 12px -2px rgb(0 0 0 / 0.05)",
      },
      animation: {
        "pulse-fast": "pulse 1s cubic-bezier(0.4, 0, 0.6, 1) infinite",
      },
    },
  },
};`;
};

For example in issue 2, I think its fine to allow people to pass icon as jsx string? And then in .tempalte.mjs we could just massage simplify like this :

Header.template.mjs :
const exampleMenuObjects = [
  {
    label: "EIP-712",
    href: "/eip-712",
    icon: `<DocumentCheckIcon className="h-4 w-4" />`,
  },
  {
    label: "EIP-712",
    href: "/eip-712",
    icon: `<DocumentCheckIcon className="h-4 w-4" />`,
  },
];

const contents = ({ menuIconImports, menuObjects }) => {
  const massageMenuObjects = exampleMenuObjects.map(obj => {
    return `{
  label: "${obj.label}",
  href: "${obj.href}",
  icon: ${obj.icon},
    }`;
  });

  console.log(massageMenuObjects);
  
  return `
        export const menuLinks: HeaderMenuLink[] = [
        {
          label: "Home",
          href: "/",
        },
        {
          label: "Debug Contracts",
          href: "/debug",
          icon: <BugAntIcon className="h-4 w-4" />,
        },
        ${massageMenuObjects.join(",\n")},
        ${menuObjects.filter(Boolean).join(",\n  ")}
      ];
    `
}

Regarding issue 3 I think it can be done now too? What if you do like this:

hardhat.config.ts.args.mjs :
export const networks = `
    myFakeNetwork: {
      url: \`https://my-fake-network.alchemyapi.io/v2/\${providerApiKey}\`,
      accounts: ["0xmyPK"],
    },
    myFakeNetwork2: {
      url: \`https://my-fake-network2.alchemyapi.io/v2/\${providerApiKey}\`,
      accounts: ["0xmyPK"],
    }`;

or I missed the point in issue3 probably you were mentioning something different?

Issue 4 again could be solved if scope our massaging in that .template.mjs itself?

All in all maybe feels like doing lot of work in .template.mjs files but since we are allowing limited things its worth or it will grow exponentially?

also I think regarding choosing when should allow people string and when object maybe it depends on that particular file and how ergonomic it is for people to copy past that code from instance to their .args.mjs file for example passing lightTheme as object or extendTheme make sense instead of wrapping them in strings and for code snippet like providerSetups in ScaffoldEthAppProvider string make sense to paste inside a string?

Lol if I think maybe our current main is just find ergonomically like peopple could just just open quotes and paste in their code as args which is not that bad either just when you look at it, it looks bad?

@rin-st
Copy link
Member

rin-st commented Sep 16, 2024

We tell people to pass objects where syntax wise its intuitive(tailwind conf, menuObjects, etc..) and strings for example passing code snippets (example in layout.template.mjs)

How, using comments? For example if by default object is empty. Because if string will be passed everything will break.

For example in issue 2, I think its fine to allow people to pass icon as jsx string? And then in .tempalte.mjs we could just massage simplify like this :

Yes, but we need to pass JSX or any variables as strings anyway

or I missed the point in issue3 probably you were mentioning something different?

export const networks = `
    myFakeNetwork: {
      url: \`https://my-fake-network.alchemyapi.io/v2/\${providerApiKey}\`,
      accounts: ["0xmyPK"],
    },
    myFakeNetwork2: {
      url: \`https://my-fake-network2.alchemyapi.io/v2/\${providerApiKey}\`,
      accounts: ["0xmyPK"],
    }`;

That private keys will be in result hardhat.config.js and will leak. We need to pass it as string again
accounts: "[deployerPrivateKey]"

To sum up, as I understand you mean the same as I wrote here #116 (comment) . We can just not change anything, just take object and handle it inside template. But if using objects we need to always pass variables as strings. So passing objects doesn't add so much value.

@rin-st
Copy link
Member

rin-st commented Sep 16, 2024

#118 adds possibility to use both strings and objects, but that variables in objects need to be stringified anyway, so regarding objects it's the same. But it adds complexity, because we're handling both cases. If we find a way to tell users what we need to use, object or string, then agree, we don't need that pr at all. And yes, we can handle both string and object in every template without stringifying in processTemplateFiles but again, it's additional complexity

@technophile-04
Copy link
Collaborator

technophile-04 commented Sep 18, 2024

To sum up, as I understand you mean the same as I wrote here #116 (comment)

Yess lol! and #109 (comment) works out box
🙌

If we find a way to tell users what we need to use, object or string, then agree, we don't need that pr at all

The way we are doing it currently is through TEMPLATE_FILES.md tell them to look up at correponding args.mjs file. So yeah that why was thinking in #116 (comment) we could allow them pass object where its intuitive and easy to copy-paste.

@rin-st
Copy link
Member

rin-st commented Oct 3, 2024

The way we are doing it currently is through TEMPLATE_FILES.md tell them to look up at correponding args.mjs file. So yeah that why was thinking in #116 (comment) we could allow them pass object where its intuitive and easy to copy-paste.

So, closing issue and pr?

  • we will use TEMPLATE_FILES.md to tell users what to use for args
  • we will allow just use objects and handle them inside templates

Is it right?

@technophile-04
Copy link
Collaborator

Closing this thanks all for the discussion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Further information is discussed
Projects
None yet
Development

No branches or pull requests

4 participants