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

Add template for tsconfig.json + getMetadata.ts + manifest.json #145

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

portdeveloper
Copy link
Collaborator

@portdeveloper portdeveloper commented Oct 26, 2024

Description

Added a template for tsconfig.json file to the project templates.

Changes

  • Added tsconfig.json template file, removed tsconfig.json

Testing

  • Tested template generation with new project creation

Here is a repo with all the changed file args
https://github.com/portdeveloper/test-everything

To-Do

  • Update nextjs config template to work with pwa

@technophile-04
Copy link
Collaborator

Hey port can you share the repo(extension) where you have the .args.mjs?

@rin-st
Copy link
Member

rin-st commented Oct 27, 2024

Could you please take a look at this table? We added it recently. Looks like you can use a raw object instead of a stringified and maybe simplify some logic

@portdeveloper
Copy link
Collaborator Author

portdeveloper commented Oct 27, 2024

Hey thanks for the comments, and I somehow did not receive a notif about Shiv's comment.

So here is the repo:
https://github.com/portdeveloper/test-tsconfig-ext

Maybe we should remove include, exclude and paths and keep the other two?
I tried using stringify but that made the process so complicated :/

lol this was difficult, lmk if i did anything wrong here

@rin-st
Copy link
Member

rin-st commented Oct 28, 2024

So here is the repo:
https://github.com/portdeveloper/test-tsconfig-ext

Update it please, params of extension are different from this pr

I tried using stringify but that made the process so complicated

Thinking without checking, but I think this should work

`${stringify({
  "compilerOptions": {
    ...
    "plugins": [
      {
        name: "next"
      },
      ...extraPlugins[0]
    ],
    ...extraCompilerOptions[0]
  },
  ...
  })}`

where extraPlugins is array of plugins and extraCompilerOptions is an object with extra options. Default values is empty array and empty object respectively

@portdeveloper
Copy link
Collaborator Author

Thanks! Could you please check again to see if i got it right? @rin-st

I have also updated the example args file

@rin-st
Copy link
Member

rin-st commented Oct 28, 2024

Thanks @portdeveloper ! Added some comments.

Also, you can test resulting template for example with this defaults

export default withDefaults(contents, {
  extraPlugins: [{name: "next-abcd"}],
  extraCompilerOptions: {abc: "def"}
})

Copy link
Member

@rin-st rin-st left a comment

Choose a reason for hiding this comment

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

Lgtm! But don't merge it for now, let's wait also Shiv or Carlos

my-tsconfig-test Outdated Show resolved Hide resolved
pwa Outdated Show resolved Hide resolved
Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

Umm @portdeveloper I think you forgot to update https://github.com/portdeveloper/test-tsconfig-ext/blob/main/extension/packages/nextjs/tsconfig.json.args.mjs ?

I am just trying to understand what extra tsConfig things we need and maybe we can set them as default in main SE-2 (for eg last time we switched "moduleResolution": "Bundler" by default in SE-2 for better working with all extensions)

Also I think you are working on pwa extension right, so maybe can you add all the required .template.mjs in this PR itself?

@portdeveloper
Copy link
Collaborator Author

@technophile-04
We need this for serwist to work!
image

And there doesn't seem to be anything an extension-builder can add besides more plugins and more compiler options?

I have updated https://github.com/portdeveloper/test-tsconfig-ext/blob/main/extension/packages/nextjs/tsconfig.json.args.mjs too

@technophile-04
Copy link
Collaborator

And there doesn't seem to be anything an extension-builder can add besides more plugins and more compiler options?

yeah true lol there not other options to add, also for pwa do you need any other more template file like manfiest.json or it was just this file?

@portdeveloper
Copy link
Collaborator Author

portdeveloper commented Oct 31, 2024

@technophile-04

We also need these:

Should I get the manifest.json and getMetadata in this PR as well?

@technophile-04
Copy link
Collaborator

technophile-04 commented Nov 1, 2024

Should I get the manifest.json and getMetadata in this PR as well?

Yes could you do that please?

Regarding this :

Change the nextjs template so that we can put things outside of the nextConfig object as in the code below:
https://github.com/portdeveloper/scaffold-pwa/blob/main/packages/nextjs/next.config.mjs

I think we need to update main SE-2 repo with next.config.js => next.config.mjs then we make the PR here to add templatise that file

Thanks @portdeveloper!!

@portdeveloper
Copy link
Collaborator Author

portdeveloper commented Nov 1, 2024

I think we need to update main SE-2 repo with next.config.js => next.config.mjs then we make the PR here to add templatise that file

Sorry for the continued confusion on this.

I think that won't be necessary! Because I am using the next.config.js version in this template!

Let me quickly update that as well.

https://serwist.pages.dev/docs/next/getting-started
image

@technophile-04
Copy link
Collaborator

technophile-04 commented Nov 1, 2024

Oh I got it wrong then sorry : P

image
The latest nextjs uses a next.config.ts.

I wonder if that's also an option for us as well?

@technophile-04
Copy link
Collaborator

The latest nextjs uses a next.config.ts.

I wonder if that's also an option for us as well?

Ohh I see! I think last time I tried using .ts in SE-2 and it gave me weird error but might be fixed and we should use .ts if it works now 🙌 Lets tinker on SE-2 main repo

PS: I am replying to my own comment because Port replied to og comment by editing it 🤣

@portdeveloper
Copy link
Collaborator Author

The latest nextjs uses a next.config.ts.

I wonder if that's also an option for us as well?

Ohh I see! I think last time I tried using .ts in SE-2 and it gave me weird error but might be fixed and we should use .ts if it works now 🙌 Lets tinker on SE-2 main repo

PS: I am replying to my own comment because Port replied to og comment by editing it 🤣

I am never gonna work under the sun again :(((((( i was like why does it say technophile there this should be a bug lmaooo

@portdeveloper
Copy link
Collaborator Author

Hey just added a commit for the manifest.json file, and hopefully I am not editing anyone's message hehe.

args.mjs i used to test things:
https://github.com/portdeveloper/manifest-test/blob/master/extension/packages/nextjs/public/manifest.json.args.mjs

Had to make iconPath optional as that is not needed in the pwa template.

@portdeveloper
Copy link
Collaborator Author

portdeveloper commented Nov 1, 2024

Honestly, I don't understand how to templatize getMetadata.ts and how to use stringify. I am so confused lol
I did it in a way but most probably not in a good way:
please correct me if it's wrong

@@ -45,6 +48,15 @@ export const getMetadata = ({
},
icons: {
icon: [{ url: "/favicon.png", sizes: "32x32", type: "image/png" }],
${extraIcons[0] ? Object.entries(extraIcons[0]).map(([key, value]) => `${key}: ${JSON.stringify(value)}`).join(',\n ') : ''}
Copy link
Member

Choose a reason for hiding this comment

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

have you tried ...${stringify(extraIcons[0]} here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does not work since there is no object there, the "..." just gets transferred to the created 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 think this its fine with current logic, there doesn't seem any good way with stringify :(, so let's use this for now

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also maybe let's also just this (alternate) solution below this row in the
Screenshot 2024-11-30 at 10 54 47 PM

# Things worth mentioning

And mention about this discussion their

@portdeveloper portdeveloper changed the title Add template for tsconfig.json Add template for tsconfig.json + getMetadata.ts + manifest.json Nov 21, 2024
Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

Thanks @portdeveloper!! Can you create the PR for pwa on create-eth-extensions so that we could test this branch as well as that PR and then we merge this PR?

Saying this because we try to follow this process when adding .template.mjs like we have corresponding PR for extension.

Now when I do :

yarn cli -e portdeveloper/test-everything test-port

And in the generated instance if I do yarn next:check-types I get some ts-errors.

Maybe a good way will be create a PWA extensions to create-eth-extensions and test this PR too and also add more necessary template.mjs files required in this PR

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