Convert to V2 Addon, drop support for ember-source < v4 #476
Conversation
NullVoxPopuli
force-pushed
the
v2-addon
branch
3 times, most recently
from
df00c0a to
20fb294
Compare
NullVoxPopuli
force-pushed
the
v2-addon
branch
from
f427af0 to
24fddc5
Compare
NullVoxPopuli
force-pushed
the
v2-addon
branch
2 times, most recently
from
3acdb40 to
7cf3fb8
Compare
addon/types/custom-ember-debug.d.ts
Outdated
There was a problem hiding this comment.
no longer needed, due to using build in types
addon/types/dummy/index.d.ts
Outdated
There was a problem hiding this comment.
no longer needed, due to using build in types
addon/types/qunit/index.d.ts
Outdated
There was a problem hiding this comment.
no longer needed, due to using build in types
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| const getChannelURL = require('ember-source-channel-url'); | |||
|
|
|||
| module.exports = async function() { | |||
| module.exports = async function () { | |||
There was a problem hiding this comment.
new prettier settings (not changeable)
NullVoxPopuli
force-pushed
the
v2-addon
branch
from
7cf3fb8 to
342b449
Compare
| @@ -4,7 +4,6 @@ on: | |||
| push: | |||
| branches: | |||
| - master | |||
| - 'v*' | |||
There was a problem hiding this comment.
we're not releasing on tag-pushes as we have a mass migration to release-plan which tags as a part of releasing, rather than releasing because of a tag
| - run: pnpm lint | ||
|
|
||
| test: | ||
| timeout-minutes: 10 | ||
| name: "Tests: ${{ matrix.EMBROIDER && 'embroider' || 'classic' }}" |
There was a problem hiding this comment.
there is no longer any build behavior, so we aren't testing embroider and classic in a matrix -- we expect that embroider handles any behaviors used by this library
| @@ -108,16 +90,12 @@ jobs: | |||
| - test-apps/base-tests | |||
| ember-try-scenario: | |||
There was a problem hiding this comment.
to cut down on CI noise, this is now just testing the ranges of support,
3.16 - minimum 4.0 minimum
x.latest - last LTS for a major, X
the 3 variable embers
There was a problem hiding this comment.
@NullVoxPopuli looks like some kind of mistake as now it only has 4.0, 4.12 and 5.12, no more 3.16 and no 3.28?
There was a problem hiding this comment.
this comment is out of date actually -- we cannot support ember < v4
NullVoxPopuli
force-pushed
the
v2-addon
branch
from
342b449 to
d5b3362
Compare
addon/.npmignore
Outdated
There was a problem hiding this comment.
replaced by package.json#files
NullVoxPopuli
force-pushed
the
v2-addon
branch
2 times, most recently
from
c582fbd to
1fcb179
Compare
| "types": "./declarations/index.d.ts", | ||
| "default": "./dist/index.js" | ||
| }, | ||
| "./__private__/types": { |
There was a problem hiding this comment.
used by tests only. there were no import restrictions in the prior version of test-waiters.
NullVoxPopuli
force-pushed
the
v2-addon
branch
from
1fcb179 to
f526251
Compare
| @@ -1,4 +1,4 @@ | |||
| import { PendingWaiterState, Waiter, WaiterName } from './types'; | |||
There was a problem hiding this comment.
using type imports is important for not shipping too much stuff to the runtime (especially stuff that doesn't exist (types-only imports don't exist at runtime, and would error otherwise))
| "start:addon": "pnpm --filter @ember/test-waiters start -- --no-watch.clearScreen", | ||
| "start:test-app": "pnpm --filter test-app start", | ||
| "test": "pnpm --filter '*' test" | ||
| }, | ||
| "devDependencies": { | ||
| "@glint/core": "^1.5.0", |
There was a problem hiding this comment.
because of vscode reasons, some LSP/Tooling dependencies need to be added to the root package.json.
No CLI tool cares that these are here
NullVoxPopuli
force-pushed
the
v2-addon
branch
from
f526251 to
0a413e5
Compare
…test-waiters, and this relationship requires ember-auto-import and test-helpers v3 has auto-import
NullVoxPopuli
changed the title
| @@ -19,47 +18,41 @@ jobs: | |||
| - uses: actions/checkout@v4 | |||
| - uses: wyvox/action-setup-pnpm@v3 | |||
There was a problem hiding this comment.
why not sync up with blueprint to use pnpm/action-setup and actions/setup-node?
There was a problem hiding this comment.
less to think about, tbh.
also also back when I made action-setup-pnpm, both setup-node and pnpm/action-setup did not infer either of their versions from the correct places.
Today the difference is (per job) 1 line vs ~ 4, I think, not too bad
| @@ -108,16 +90,12 @@ jobs: | |||
| - test-apps/base-tests | |||
| ember-try-scenario: | |||
There was a problem hiding this comment.
@NullVoxPopuli looks like some kind of mistake as now it only has 4.0, 4.12 and 5.12, no more 3.16 and no 3.28?
| "files": [ | ||
| "declarations", | ||
| "dist", | ||
| "src", |
There was a problem hiding this comment.
@NullVoxPopuli why would we need src to be published?
There was a problem hiding this comment.
so go-to-definition can go to source instead of declarations (requires declaration maps, iirc)
| "globals": "^15.12.0", | ||
| "loader.js": "^4.7.0", | ||
| "execa": "^9.5.1", | ||
| "fix-bad-declaration-output": "^1.1.4", |
There was a problem hiding this comment.
it's a default thing I use in v2 addons, because the /// <reference thing with peers is too annoying to fix otherwise if it comes up
There was a problem hiding this comment.
ok, so it's just in case here rather actually needed?
There was a problem hiding this comment.
Idk. It is used in the rollup config.
It's just not a problem i want to run in to, since TS is very against properly working peers - there isn't a better path forward:(
There was a problem hiding this comment.
thanks for explanation! IMO it's ok to keep it as long as any future reader can easily understand/use it which should be the case here
There was a problem hiding this comment.
yea, in the rollup config I link to some discussion on the TS repo about peer-provided types
| @@ -128,7 +128,7 @@ class NoopTestWaiter implements TestWaiter { | |||
| /** | |||
| * Builds and returns a test waiter. The type of the | |||
| * returned waiter is dependent on whether the app or | |||
| * addon is in `isDevopingApp()` mode or not. | |||
There was a problem hiding this comment.
I may like to Dev Oping, more like Dev Oopsing lol
| @@ -13,7 +13,7 @@ import { module, test } from 'qunit'; | |||
|
|
|||
| // @ts-ignore | |||
| import { Promise } from 'rsvp'; | |||
| import Token from '@ember/test-waiters/token'; | |||
| import Token from '@ember/test-waiters/__private__/token'; | |||
There was a problem hiding this comment.
how does this extra /__private__/ works?
There was a problem hiding this comment.
I think that this is not supposed to be true and if it's true it's a bug in ember-auto-import. We try to maintain compatibility so that all v2 addons are always available to every package, in keeping with the classic behavior. |
|
so far, in the auto-import test suite: I'm unable to create a rero 🤔 https://github.com/embroider-build/ember-auto-import/pull/650/files maybe everything is fine then! |
|
removed the second bullet -- the only requirement for v1 addons is that they have ember-auto-import -- as long as they do, folks should be able to use the new/upcoming version of |
Applies https://github.com/embroider-build/addon-blueprint
+
some updates from ember-cli 6.1 (better eslint)
Marked as breaking because
@ember/test-waitersError: @ember/test-helpers needs to depend on ember-auto-import in order to use @ember/test-waiters@ember/test-helpersv3 has ember-auto-import@ember/test-helpersv2 does not have ember-auto-import@ember/test-waitersComparing with v3: