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

Fix support for --typescript #80

Merged
merged 6 commits into from
Nov 11, 2024
Merged

Fix support for --typescript #80

merged 6 commits into from
Nov 11, 2024

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented Sep 23, 2024

For testing this out (without cloning), I published this branch under @nullvoxpopuli/ember-vite-app

So, creating a TS app would happen like:

npx ember-cli@latest new my-app-name \
  --blueprint @nullvoxpopuli/ember-vite-app \
  --pnpm \
  --typescript

Extractions

The upstream blueprint supports --typescript, and a fair number of folks wanting "to try out the Vite blueprint" want to use TypeScript.

The effort required here isn't huge, mostly config changes.

  • get TS working
    • support hbs and the template-registry (should this be opt in/out?) will happen in another PR
  • remove/convert TS files for the JS output

The Approach:

  • split all files by js/ts based on presence of the --typescript option
    • this makes it easier to maintain each set of files, and compare them
    • previous blueprints trying to inline all the conditions for typescript vs not typescript have proven tricky to maintain, and harder to read than they otherwise could be! (like in this PR!)
    • this also allows easier separation since proper TS support is very invasive to a project -- a lot needs to change

What isn't implemented:

  • types registry (for loose mode)
  • service registry
  • models registry

These will show up in future PRs (as this one is already quite big) -- and how we want to teach ember-data in the future also doesn't include models, so I'll need to sink with @runspired on what the strategy should be there (or if ember-data/warp-drive has its own generator, etc (since it's meant to be very "use what you need"))

@NullVoxPopuli NullVoxPopuli added the bug Something isn't working label Sep 23, 2024
index.js Outdated Show resolved Hide resolved
@NullVoxPopuli NullVoxPopuli marked this pull request as ready for review September 24, 2024 22:48
Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

Lots of little comments, not all of them are blocking but some of them are. Let me know if you have any questions about these 👍

files-override/js/.eslintrc.cjs Outdated Show resolved Hide resolved
files-override/shared/.prettierrc.cjs Outdated Show resolved Hide resolved
files-override/ts/app/app.ts Outdated Show resolved Hide resolved
files-override/ts/app/app.ts Outdated Show resolved Hide resolved
files-override/ts/tests/test-helper.ts Show resolved Hide resolved
tests/default.test.mjs Outdated Show resolved Hide resolved
tests/default.test.mjs Outdated Show resolved Hide resolved
tests/fixture-ts/tests/acceptance/welcome-page-test.js Outdated Show resolved Hide resolved
tests/typescript.test.mjs Outdated Show resolved Hide resolved
tests/typescript.test.mjs Show resolved Hide resolved
@simonihmig
Copy link
Contributor

@NullVoxPopuli This is a bit hard to review now with all the stuff included that has been extracted into their own PRs. Could you please ping us again, when those extractions have been merged and this rebased?

Thanks for working on this, this is much needed! (for some people at least 🙂 )

@NullVoxPopuli
Copy link
Collaborator Author

of course yes! I don't know how to un-ping people once I put a PR back in to draft status 🙈

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
Base automatically changed from extract-helpers to main September 30, 2024 09:27
},
};

export default ts.config(
Copy link
Collaborator Author

@NullVoxPopuli NullVoxPopuli Sep 30, 2024

Choose a reason for hiding this comment

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

this utility is neat -- it re-adds some basic support for extends
-- its kind of a less-powerful version of configFor + forFiles

Here is proof this config works:
image

image

@NullVoxPopuli NullVoxPopuli dismissed mansona’s stale review October 1, 2024 16:53

This PR is entirely different now

@@ -2,6 +2,7 @@

module.exports = {
plugins: ['prettier-plugin-ember-template-tag'],
singleQuote: true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

without this change, the vite config in this folder is changed to double quotes...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

feels like a prettier bug, tbh, given line 10 here

Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

still a few things left to fix 👍

files/ts/app/templates/application.gts Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

I have only one more thing that I am not sure about with this PR, it's not a blocker but I think we need to be careful here because stylelint has broken on us in the past

index.js Outdated Show resolved Hide resolved
Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

I was just going to merge this PR and have done with it but I saw there were conflicts so can we fix these last few issues and resolve the conflict and we can get this merged 👍

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@Alonski
Copy link

Alonski commented Nov 10, 2024

@NullVoxPopuli Can I help getting this merged please?

Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

👍

@mansona mansona added enhancement New feature or request and removed bug Something isn't working labels Nov 11, 2024
@mansona mansona merged commit 6a8e4cf into main Nov 11, 2024
2 of 3 checks passed
@mansona mansona deleted the fix-typescript branch November 11, 2024 10:08
@github-actions github-actions bot mentioned this pull request Nov 5, 2024
@@ -2,4 +2,12 @@

module.exports = {
singleQuote: true,
overrides: [
{
files: ['*.js', '*.ts', '*.cjs', '.mjs', '.cts', '.mts', '.cts'],

Choose a reason for hiding this comment

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

.cts is in here twice, and it seems some entries are missing the * character?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ope

Copy link
Member

Choose a reason for hiding this comment

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

I was going to point out that overrides are overriding what is already being set as the default on line 4 so this whole block is pointless (I think) 🙈

this PR had too much back and forth I just let that one slide

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Iteration is easier

Ye

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants