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 smoke-tests for types, build, etc #9633

Merged
merged 14 commits into from
Jan 29, 2025
Merged

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Jan 9, 2025

Minimal reproduction of the problem I'm addressing: https://github.com/NullVoxPopuli/ts-types-package-json-exports-proof-for-ember-data

Description

Supersedes: #9631
Resolves: #9630

ember-data / warp-drive has enough going on it to warrant testing if new projects in various scenarios will work:

  • linting
  • type checking
  • building
  • a basic test

Unblocks:

Notes for the release

fix: tsconfig.json#compilerOptions#types can now use the package.json#exports paths, example:

"compilerOptions": {
  "types": [
      "ember-source/types",
      "ember-data/unstable-preview-types",
      "@ember-data/store/unstable-preview-types",
      "@ember-data/adapter/unstable-preview-types",
      "@ember-data/graph/unstable-preview-types",
      "@ember-data/json-api/unstable-preview-types",
      "@ember-data/legacy-compat/unstable-preview-types",
      "@ember-data/request/unstable-preview-types",
      "@ember-data/request-utils/unstable-preview-types",
      "@ember-data/model/unstable-preview-types",
      "@ember-data/serializer/unstable-preview-types",
      "@ember-data/tracking/unstable-preview-types",
      "@warp-drive/core-types/unstable-preview-types"
  ]
}

Notes for review

NPM is not as understanding when it comes to tarballs:

npm error code ERESOLVE
npm error ERESOLVE could not resolve
npm error
npm error While resolving: @ember-data/[email protected]
npm error Found: @ember-data/[email protected]
npm error node_modules/@ember-data/store
npm error   dev @ember-data/store@"file:./ember-data-store-5.4.0-alpha.126.tgz" from the root project
npm error   peer @ember-data/store@"*" from @ember-data/[email protected]
npm error   node_modules/@ember-data/adapter
npm error     dev @ember-data/adapter@"file:./ember-data-adapter-5.4.0-alpha.126.tgz" from the root project
npm error     @ember-data/adapter@"*" from [email protected]
npm error     node_modules/ember-data
npm error       dev ember-data@"file:./ember-data-5.4.0-alpha.126.tgz" from the root project
npm error   7 more (@ember-data/debug, @ember-data/graph, ...)
npm error
npm error Could not resolve dependency:
npm error peer @ember-data/store@"^4.12.0 || ^5.0.0" from @ember-data/[email protected]
npm error node_modules/@ember-data/rest
npm error   dev @ember-data/rest@"file:./ember-data-rest-5.4.0-alpha.126.tgz" from the root project
npm error
npm error Conflicting peer dependency: @ember-data/[email protected]
npm error node_modules/@ember-data/store
npm error   peer @ember-data/store@"^4.12.0 || ^5.0.0" from @ember-data/[email protected]
npm error   node_modules/@ember-data/rest
npm error     dev @ember-data/rest@"file:./ember-data-rest-5.4.0-alpha.126.tgz" from the root project

the install-time peer checking here isn't as important as the post-install peer checking -- we don't want npm using the versions from package.json exactly, but the versions installed the specifiers in the package.json -- kinda surprised they have this bug, tbh

@NullVoxPopuli NullVoxPopuli changed the base branch from main to fix-publish January 11, 2025 22:51
@NullVoxPopuli NullVoxPopuli force-pushed the smoke-test-with-non-pnpm branch from 9cc348f to 7b0eda0 Compare January 11, 2025 23:03
Base automatically changed from fix-publish to main January 14, 2025 20:12
@NullVoxPopuli NullVoxPopuli force-pushed the smoke-test-with-non-pnpm branch 2 times, most recently from fb5e4d5 to d1de3f5 Compare January 14, 2025 22:22
.npmrc Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without these changes not all node_modules directories have the right bins in node_modules -- for example, @warp-drive/ember did not have glint, and would fail to build its types

@NullVoxPopuli NullVoxPopuli force-pushed the smoke-test-with-non-pnpm branch from 27208ad to bed0680 Compare January 22, 2025 15:19
Copy link
Contributor

Choose a reason for hiding this comment

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

seems odd for these to have been removed ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

likely: a minor lint update / transitive dep changed the meaning of what needs to be disabled.
now: the lint-disable is only needed on the actual real implementation, not the overrides/declarations

Copy link
Contributor

Choose a reason for hiding this comment

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

why was this needed?

private declare _isCacheHandler: boolean;
private declare _finalized: boolean;
declare private _isCacheHandler: boolean;
declare private _finalized: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

as far as I can tell TS doesn't specify the order on this nor care https://tsplay.dev/WJKaRw

declare-first does seem better though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lint:fix did this

@@ -0,0 +1,34 @@
/**
* These tests are more fro the out-of-monorepo tests,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* These tests are more fro the out-of-monorepo tests,
* These tests are more for the out-of-monorepo tests,

/**
* TODO: find another way to set this config,
* because the most modern of apps will not have a compat build
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

once embroider/macros picks a new key that's all we need. app and __dirname here are just for the macros API that needs them.

Copy link
Contributor

Choose a reason for hiding this comment

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

(probably should remove this comment though)

@@ -13,6 +11,8 @@ module('it works', function (hooks) {
});

test('we can use the store', async function (assert) {
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore this repo loads the declarations for qunit in the wrong order
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// @ts-ignore this repo loads the declarations for qunit in the wrong order
// @ts-expect-error this repo loads the declarations for qunit in the wrong order

@@ -17,7 +17,7 @@
"scripts": {
"build:tests": "IS_TESTING=true EMBER_CLI_TEST_COMMAND=true ember build --output-path=dist-test --suppress-sizes",
"_build:production": "bun run build:tests -e production",
"lint": "eslint . --quiet --cache --cache-strategy=content",
Copy link
Contributor

Choose a reason for hiding this comment

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

accidental check-in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nay -- eslint's cache being buggy is why ci and editor and cli are sometimes not in agreement

@runspired runspired added 🎯 canary PR is targeting canary (default) 🏷️ bug This PR primarily fixes a reported issue labels Jan 29, 2025
@runspired runspired self-assigned this Jan 29, 2025
@runspired runspired merged commit 3a720ae into main Jan 29, 2025
29 checks passed
@runspired runspired deleted the smoke-test-with-non-pnpm branch January 29, 2025 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎯 canary PR is targeting canary (default) 🏷️ bug This PR primarily fixes a reported issue
Projects
Archived in project
2 participants