Skip to content

Support scenario testing #41

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Support scenario testing #41

wants to merge 10 commits into from

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented May 19, 2025

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.

very close! let me know if you have any questions about this review and we can hop on a quick call 👍

devDependencies: {
'ember-source': emberVersion,
'@embroider/compat': '^4.0.3',
'ember-cli': '^5.12.0',
Copy link
Member

Choose a reason for hiding this comment

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

I understand why we're doing this because we need some ember-cli in the old versions but I don't think we benefit all that munch from having functions generate each of these scenarios. Can we just split it out to objects in the senarios array which will have a much clearer upgrade path for people used to ember-try?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why is that clearer?

with the functions, it's like... compatibility codified.
with "just objects", it applies every scenario has different compatibility requirements -- which isn't true

Copy link
Member

Choose a reason for hiding this comment

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

well it is true depending on how far back you go 🤔 as you go back in the versions you need to make more and more changes e.g. https://github.com/mansona/ember-cli-notifications/blob/main/test-app/config/ember-try.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should have some like.. shared database of these things somewhere -- centralized or something, in a library

Copy link
Member

Choose a reason for hiding this comment

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

yea I tried to add it to ember-try a while back but we're now off on a different path with @embroider/try

@NullVoxPopuli NullVoxPopuli dismissed mansona’s stale review May 29, 2025 18:34

addressed comments

@NullVoxPopuli
Copy link
Contributor Author

NOTE:
custom exportConditions won't work, because of this problem: https://stackblitz.com/edit/stackblitz-starters-4ydzmcht?description=Starter%20project%20for%20Node.js,%20a%20JavaScript%20runtime%20built%20on%20Chrome%27s%20V8%20JavaScript%20engine&file=pkg%2Fpackage.json,pkg%2Findex.js,package.json,index.js,pnpm-workspace.yaml,reversed.js&title=node.new%20Starter
which presents itself in vite build and vite dev

The options that were attempted for resolving self-imports:

  • custom resolve alias (requires config in both vite and tsconfig)
  • custom exportConditions - only works if you always specify extensions (no array / fallback lookups like in the stackblitz repro)
  • ignoring self-imports and doing subpath imports (e.g.: PR Add package.json#imports #47)

We've discussed that for a docs/demo app, we'd still want the code to look like it would to a consumer. We can do that via custom resolves in vite and tsconfig, but we should make that a separate concern from this PR. Using subpath imports for testing is good, and is fine since testing can have a lot of not-so-common situations to begin with.

@lupestro
Copy link

lupestro commented Jun 3, 2025

Silly nit - node 18 still? I suppose this is going back a way, so maybe that makes sense.

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.

3 participants