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 note for workaround for --typescript issues #123

Closed
wants to merge 3 commits into from

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented May 21, 2023

tl;dr:


People are using the v2 addon blueprint, and when trying to use --typescript, there are undocumented / hard to find issues with how --typescript works in ember-cli right now.

This README updates documents some of those, and adds some manual instructions so folks get be unblocked.
(It's important to unblock folks while we work on a proper solution, as folks want to do what they want to do, and we/I need to enable them 🎉 )

There are a few changes in ember-cli-typescript that could probably help this situation, but also, I don't blame anyone for not staying on top of it -- better changes for TypeScript are just around the corner, and effort is better (and has been!) spent there.

https://github.com/typed-ember/ember-cli-typescript/commits/master

Ultimately, I think the best thing for folks to do if they are starting new projects is to use the preview types. They are so much a better experience, that I can't believe we've used the types from DT for so long. The typed-ember team has done an amazing job on making TS a native experience.
See this other PR for native types: #118

NullVoxPopuli added a commit to NullVoxPopuli/ember-cli-typescript that referenced this pull request May 21, 2023
…pes now.


Without this change, new projects using ember-cli-typescript have errors running `tsc`.
See also, other work-arounds: embroider-build/addon-blueprint#123
touch package.json # make sure to fill this out
touch pnpm-workspace.yaml # if you're using pnpm
npx ember-cli@latest addon my-addon -b @embroider/addon-blueprint --skip-git --skip-npm --typescript --addon-only
npx ember-cli@latest new test-app --skip-git --skip-npm --typescript # make sure to add the addon as a dependency in the test-app
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you propose this as a separate step? What does it help with?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How else do you make a v2 addon monorepo with typescript?

Copy link
Collaborator

@simonihmig simonihmig May 22, 2023

Choose a reason for hiding this comment

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

Just npx ember-cli@latest addon my-addon -b @embroider/addon-blueprint --typescript?

Copy link
Collaborator Author

@NullVoxPopuli NullVoxPopuli May 22, 2023

Choose a reason for hiding this comment

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

it's broken-by-default (with some asterisks)

Here is that command's output in StackBlitz: https://stackblitz.com/edit/node-iuvwaw
Repro:

cd my-addon
yarn

Issues I noticed:

  • using --typescript ended up using --yarn, but without teh --yarn flag so the v2 blueprint was generated assuming npm 🙈
  • $ npm run build --workspace my-addon seems to infinite loop under yarn 😅
    • had to fix this / remove prepare to get on with this demo 😅

I noticed that lint:types works with using the non-preview types under yarn.
So here is a small change to use the preview types as in #118,
https://stackblitz.com/edit/node-okq4r6?file=my-addon%2Fpackage.json,my-addon%2Fmy-addon%2Funpublished-development-types%2Findex.d.ts,my-addon%2Fmy-addon%2Fpackage.json

cd my-addon
yarn
yarn lint
# error! mismatched types

The reason this is a yarn problem is that the v2-addon's usage of ember-source's built-in types is a development concern, which the consuming app's types shouldn't care about.

The way I'm thinking of trying to solve it is to get us off of DT's types.
Part 1: #118
Part 2: make the app blueprint use native types as well? (or merge all the ec-typescript PRs -- but I'm not confident that the DT types can be made compatible with a yarn monorepo)

Here is an example of what the state of things are with the built-in types, and also with the ec-ts changes:
https://stackblitz.com/edit/node-n5yqmy?file=my-addon%2Fpackage.json,my-addon%2Fmy-addon%2Funpublished-development-types%2Findex.d.ts,my-addon%2Ftest-app%2Fpackage.json

Same issue.

However, if the generated app were to use the built in types as well: https://stackblitz.com/edit/node-3txw9q?file=my-addon%2Fpackage.json,my-addon%2Ftest-app%2Ftypes%2Findex.d.ts,my-addon%2Ftest-app%2Fpackage.json
then everything works.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

one thing that I think could hold up the default app blueprint from using native types is that I don't know of ember-data is ready for its built in types to be used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we drop support for --yarn then.. all of this is a non issue 😅
(not saying we actually do that, but like all things, dropping yarn makes everything easier. lol)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, but I am not really following...

I'd like to keep using the native types out of this discussion, as this seems a separate topic and the current blueprint should work with what we have now...

using --typescript ended up using --yarn

Ok, haven't tested this, but if this is the case, then let's fix this.

npm run build --workspace my-addon seems to infinite loop under yarn

But why are you using this command with yarn?

I did npx ember-cli@latest addon my-addon -b @embroider/addon-blueprint --typescript --yarn just now. And there is an error popping up, due to @types/ember__test-helpers being still used by the test-app. I already removed this here, but this hasn't been released yet unfortunately.

When removing @types/ember__test-helpers, then all of this just works fine for me:

  • yarn lint
  • yarn test
  • yarn build
  • yarn start

If we drop support for --yarn then.. all of this is a non issue 😅

Again, why?

Why do you propose this as a separate step? What does it help with?

You haven't really answered that question yet. What makes running those two blueprints separately better, when our blueprint does literally the same (when not using --addon-only)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to keep using the native types out of this discussion

They are entirely related though, to ignore native types means there is no issue with yarn.

Again, why?

yarn is resolving types incorrectly -- afaict, is the only package manager that can't handle native types in the addon and DT types in the test app :(

Copy link
Collaborator

Choose a reason for hiding this comment

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

Then I don't understand the scope of this PR. It is adding a note for TS users, it is not introducing preview types. This is what #118 is doing, and we can only land that one if it works correctly, including with yarn. But without preview types, --typescript --yarn is (mostly) working. So what are we doing here?

You haven't really answered that question yet. What makes running those two blueprints separately better, when our blueprint does literally the same (when not using --addon-only)

?

Copy link
Collaborator Author

@NullVoxPopuli NullVoxPopuli May 24, 2023

Choose a reason for hiding this comment

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

yeah, apologies for the confusion -- it's entirely possible I've over-corrected for being told that -- my PRs are too big and that's why folks never review any of my stuff in OSS (which is fair criticism, but splitting things up also makes it hard for folks to see the whole story of a change) -- and now this PR doesn't make sense in isolation.

It is Draft, and I've (just now) added 3 important caveats to the top.
Also!, I'm totally happy with this PR not ever merging, and only being a documentation of sorts for folks that run in to the situation that we've run in to, with the ecosystem between between native types and DT types.

@NullVoxPopuli
Copy link
Collaborator Author

It's looking like yarn@v1's nohoist on ember-source is resolving the problem. #118

Will close.

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.

2 participants