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

feat(create-quasar): add TS AE template #17168

Open
wants to merge 23 commits into
base: dev
Choose a base branch
from

Conversation

yusufkandemir
Copy link
Member

@yusufkandemir yusufkandemir commented May 6, 2024

What kind of change does this PR introduce?

  • Feature

Does this PR introduce a breaking change?

  • No

The PR fulfills these requirements:

  • It's submitted to the dev branch (or v[X] branch)

Other information:
Contains fixes for app-vite&app-webpack types of AE API files, e.g. IndexAPI

Adds a TS AE template, highlights:

  • TypeScript setup, of course
  • ESLint v9 using flat config format, with a good configuration (ESLint v9 will be backported to all other relevant templates later)
  • Vue SFC (.vue files) support (will be backported to the JS template later) (thanks @danielroe for tips)
  • Monorepo structure with app-vite and app-webpack playground apps

There is a limitation with the SFC support. When using <script setup lang="ts">, it won't work in JS app-webpack apps. This is because mkdist skips processing it and there is no TypeScript support in JS app-webpack, so it fails. If unjs/mkdist#210 gets released, then this limitation will become "when using <script setup lang="ts"> AND type-only macros, e.g. defineProps<QBtnProps>().
Alternatively, to completely eliminate the limitation, we could add TS processing support to app-webpack when using JS. We may do this in the core, or via a snippet in AE index API. app-vite JS works fine due to how Vite is configured to handle TS automatically.
For now, I just left some comments explaining the limitations.

What to do next:

I haven't added the UI kit due to complexity, limitations, time constraints, etc. It can be plugged into the template in the form of another workspace entry in the monorepo later. As the AE can now also ship SFC files and act as a library that can expose methods and such, the importance of the UI kit is reduced. The only purpose of the UI kit is to support other flavors, e.g. Vite plugin, Vue CLI plugin, and UMD. UMD will be problematic regarding SFC support as AEs will ship transpiled .vue files and there is no build system to integrate those. We may need to build SFCs into .js and ship it that way for UMD, but we would need to figure out the limitations to do that.

We may also want to offer the non-monorepo version of the TS AE kit as people might want to create AEs into their existing monorepos.

Copy link
Member

@IlCallo IlCallo left a comment

Choose a reason for hiding this comment

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

.gitignore file is missing into templates/app-extension/ae-ts/BASE

Example content:

.DS_Store
.thumbs.db
node_modules
dist

# Log files
npm-debug.log*
yarn-debug.log*
yarn-error.log*

# Editor directories and files
.idea
*.suo
*.ntvs*
*.njsproj
*.sln

This is important since we now auto-initialize a Git repo when the scaffolding completes


Latest commits and in general some package.json lines assume the dev will be using pnpm

E.g. workspace:* would break installation with npm and yarn
For npm it seems that using ^0.0.1 would be fine to get past deps installation, I couldn't find a way to say "just take the latest available in the repository"

I haven't tested this with Bun

This needs to be fixed before merging

Btw, I simulated the usage of different package managers with this command, to get runningPackageManager filled in with some actual values

npm_config_user_agent=npm/10.5.0 node <path-to-quasar-folder>/quasar/create-quasar/index.js


I haven't explored how hard it would be to implement this, but we should explore at some point running the root level format command after deps installation but before linting
This should probably be done automatically and for all create-coralloy templates, if a format script actually exists, without an option to let the user choose


I went ahead, rebased the PR and fixed a couple of my own comments in my local copy
If you're ok with it, I can force-push the rebase and my commits for you


If this is fixed, I can ping Daniel about it and see if we can get it in


When running the AE build command, I got a warning from mkdist about installing vue-tsc to get proper typings for components, or something similar
Are you aware of it?

"dependencies": {
"@quasar/extras": "^1.16.11",
"quasar": "^2.15.2",
"<%= pkgName %>": "workspace:*",
Copy link
Member

Choose a reason for hiding this comment

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

Does this work with npm and yarn too?
Isn't workspace:* a pnpm thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. The template was pnpm only at first. It seems I've overlooked this part. I will think about it and do some research. yarn v1 should auto-link them. Not sure how npm does it. Should be solvable with some if-else rendering.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@IlCallo
Copy link
Member

IlCallo commented Aug 8, 2024

Check out latest commits in this branch, where I pushed some fixes for the problems I highlighted

@yusufkandemir
Copy link
Member Author

I went ahead, rebased the PR and fixed a couple of my own comments in my local copy
If you're ok with it, I can force-push the rebase and my commits for you

I already had rebased it and made some changes. They were waiting locally. So, I force-pushed them instead. Thanks. I will check the changes for other bits that I haven't addressed.

If unjs/mkdist#210 is fixed, I can ping Daniel about it and see if we can get it in

I've done it myself, thanks.

Btw, I simulated the usage of different package managers with this command, to get runningPackageManager filled in with some actual values
npm_config_user_agent=npm/10.5.0 node /quasar/create-quasar/index.js

Just FYI, pnpm node .../index.js, yarn node .../index.js, etc. also work, while inside a directory with package.json.

When running the AE build command, I got a warning from mkdist about installing vue-tsc to get proper typings for components, or something similar
Are you aware of it?

It's a recent change. I've noticed it recently, after returning for the final touches. Installing it didn't make any difference for the test component. But, I'll add vue-tsc to the template, to avoid the warning and just in case.

I haven't explored how hard it would be to implement this, but we should explore at some point running the root level format command after deps installation but before linting

We used to run the format script, but probably after listing. It was probably a very long time ago. Maybe it was lost while migrating from starter-kit to create-quasar. It wasn't really hard to do so. Running it after linting is better, when ESLint auto-fixes some stuff. For example, if there is an unused eslint-disable comment, ESLint removes the comment, leaving the line empty. Running the format script afterward, clean up those empty lines, and adjust other styles if needed. Do you have any reason why do you want to run linting before formatting?

@IlCallo
Copy link
Member

IlCallo commented Aug 12, 2024

Do you have any reason why do you want to run linting before formatting?

Honestly, your reasoning makes much more sense than my own thoughts
I probably remember wrong and linting > formatting is the correct order

@rstoenescu
Copy link
Member

AE, TS, PNPM:
image

@rstoenescu
Copy link
Member

rstoenescu commented Sep 4, 2024

AE, TS, Yarn:

  1. "To get started" section is wrong. We'll need some custom code.
  2. Picked yarn and still having pnpm stuff (pnpm-workspace, package.json > scripts > pnpm commands)

image

Copy link
Member

@rstoenescu rstoenescu left a comment

Choose a reason for hiding this comment

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

Posted two comments earlier

@yusufkandemir
Copy link
Member Author

@rstoenescu

Regarding the instructions for getting started in the CLI message, I'll check it out.

The package manager stuff is like that because it uses the detected package manager to scaffold stuff accordingly. The selection only happens after the scaffolding is complete, so we can't use it with how the code is structured now. pnpm create, npm init, etc. should all detect the package manager fine. The only case I can find is running the script directly via Node. So, in practice, I think it's fine.

So, do you think we should leave it as is? Should I rework how package manager selection happens to scaffold stuff accordingly? Should I do something else?

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.

None yet

3 participants