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

Use built in types from ember-source instead of the types-packages #118

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

NullVoxPopuli
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli commented May 16, 2023

This PR is blocked until:

  • ember new my-app --typescript uses the built in types (see discussion Add note for workaround for --typescript issues #123)
    • due to yarn@v1 not resolving types directly? this are getting weirder as I investigate this. the TS test that's failing uses both yarn and pnpm.
    • npm and pnpm can handle the app using types from DT and the addon using native types

This reduces a lot of the dependency management that folks need to do.

Best of all, this change shouldn't affect consumers of existing v2 addons as there are developement-only types.
(with the caveat that some of the types from DT may not be compatible with the built-in ember-source types)

@@ -36,31 +39,16 @@
"@babel/plugin-syntax-decorators": "^7.17.0",
"@babel/runtime": "^7.17.0",
"@embroider/addon-dev": "^3.0.0",<% if (typescript) { %>
"@glimmer/component": "^1.1.2",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is required by glint

@@ -25,6 +25,9 @@
"test": "echo 'A v2 addon does not have tests, run tests in test-app'",
"prepack": "rollup --config"
},
"peerDependencies": {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

highly likely a v2 addon will want a peer on ember-source

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems good to me, especially since the v1 blueprint also has this, but I remember this comment by @ef4 🤔

the main reason people are forced to add it is if they want to say dependencySatisfies('ember-source', ...).

but this is the case quite often, often implicitly by others doing this, e.g. see ember-polyfills/ember-cached-decorator-polyfill#212

So I'm 👍, but just wanted to make sure...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmmm that's a good point. I'll hopefully remember to bring this up in tomorrows meeting.

@NullVoxPopuli
Copy link
Collaborator Author

The typescript test is using yarn@v1 which manages dependencies incorrectly, and gives us this error during build:

[!] (plugin Typescript) TS2688: Cannot find type definition file for 'ember__test-helpers'.
  The file is in the program because:
    Entry point for implicit type library 'ember__test-helpers'

This is with a project generated via:

❯ npx ember-cli addon ts-project -b ../OpenSource/addon-blueprint/ --skip-git --yarn --typescript

changing nothing else about ts-project

  • delete all node_modules recursively
  • delete yarn.lock
  • add pnpm-workspace.yaml that has these contents:
    packages:
    
  • test-app
  • ts-project
- run `pnpm i`
- cd `ts-project`
- run `pnpm build` (same as previous `yarn build` aka `rollup -c`) 

🎉 success

This will be a problem unless we set `nohoist` in `yarn` projects to include `ember-source` -- but I believe that'll still resolve incorrectly when it comes time for the addon developer to use ember-try.

I kinda think we should explicitly _not_ support `yarn@v1` for as long as the v2 addon blueprint is a monorepo.

(long term, I have a plan to make the v2 addon blueprint a uni-repo repo like the v1 addon blueprint is, but that's a ways off as it requires the vite work from embroider, and a special vite plugin from addon-dev that doesn't exist yet for an in-memory test-app -- ( and I think we should still support the monorepo workflow, as it's the only real way to test if a package is actually being built correctly, but I don't think we can support yarn@v1 with the monorepo layout ))

@NullVoxPopuli
Copy link
Collaborator Author

It's likely this problem will go away when the --typescript app blueprint uses the built in types.

@chriskrycho -- I remember reading you ran in to some issues promoting the preview types to stable -- do you have those listed anywhere, so I can make note of them?

@@ -25,6 +25,9 @@
"test": "echo 'A v2 addon does not have tests, run tests in test-app'",
"prepack": "rollup --config"
},
"peerDependencies": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems good to me, especially since the v1 blueprint also has this, but I remember this comment by @ef4 🤔

the main reason people are forced to add it is if they want to say dependencySatisfies('ember-source', ...).

but this is the case quite often, often implicitly by others doing this, e.g. see ember-polyfills/ember-cached-decorator-polyfill#212

So I'm 👍, but just wanted to make sure...

files/__addonLocation__/package.json Show resolved Hide resolved
files/__addonLocation__/package.json Outdated Show resolved Hide resolved
@simonihmig simonihmig added the enhancement New feature or request label May 22, 2023
@NullVoxPopuli NullVoxPopuli marked this pull request as draft May 24, 2023 15:04
@NullVoxPopuli
Copy link
Collaborator Author

Moving to draft, as I have some things to explore, and later extract from this PR.

In order to have effective use of the built in types, ember-source needs to be around 4.12, however, that's *just for development*, and actual runtime usage can be much wider

Add npmrc per pnpm RFC updates
@@ -33,33 +36,19 @@
<% if (typescript) { %>"@babel/preset-typescript": "^7.18.6"<% } else { %>"@babel/eslint-parser": "^7.19.1"<% } %>,
"@babel/plugin-proposal-class-properties": "^7.16.7",
"@babel/plugin-proposal-decorators": "^7.20.13",
"@babel/plugin-proposal-object-rest-spread": "^7.20.7",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this will go away with this PR: #136

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.

3 participants