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.

Sign up for GitHub

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
+24 −0
Closed

Add note for workaround for --typescript issues #123

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc8187b
Add note for workaround for --typescript issues
NullVoxPopuli May 21, 2023
53925d9
Update README.md
NullVoxPopuli May 21, 2023
f78a829
Update README.md
NullVoxPopuli May 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,30 @@ This is still work in progress.

The blueprint contains a number of assumptions, e.g. using a monorepo using (`yarn` or `npm`) workspaces, with separate workspaces for the addon and the test-app. But there is plenty of room for bikeshedding here, so if you have suggestions about better ways to set this up, then please file an issue to discuss!

### Temporary work-around for `--typescript` issues

Today, `--typescript` is implemented via differing to `ember-cli`'s `--typescript` when generating the test-app.
This has a number of issues:
- `ember-cli-typescript` is very out of date
- you'll need to remove `@types/ember__test-helpers`, `@types/ember-resolver`, and `@types/ember__string` (these provide their own types now)
- it's being sunset anyway, as effort is being put in to making TS support more native and less reliant on DefinitelyTyped.
- peer issues with ember-cli cause the generation of a whole project to be deleted by ember-cli
- the package manager is ignored when ember-cli defers to `ember-cli-typescript`s blueprint (this mostly only affects `--pnpm` using projects)
- there is no way to opt out of ember-cli installing dependencies when using `--typescript`

To make a monorepo manually:
```bash
mkdir my-monorepo
cd my-monorepo
git init
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

```

We're actively trying to make this situation better, but this is what we have to deal with until `--typescript` support in ember-cli is more stable (and maybe uses the built-in types).

## Usage

```bash
Expand Down