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

fix(enhanced-img): preserve ambient context for *?enhanced imports #12363

Merged
merged 2 commits into from
Jul 7, 2024

Conversation

BastiDood
Copy link
Contributor

@BastiDood BastiDood commented Jun 17, 2024

Preserving the ambient context

#12224 changes the default export of *?enhanced modules from a value of type string into a value of type Picture (from vite-imagetools). However, this change caused the ambient.d.ts file to no longer be an ambient module. According to the TypeScript docs, top-level imports and exports automatically upgrade the ambient context into a module context. That means the declare module '*?enhanced' is no longer applied "ambiently", but now requires an explicit import somehow. This breaks svelte-check and other static analysis tooling because TypeScript would report that any module imported as *?enhanced have no existing type declarations.

The fix is quite simple: simply move the top-level import into the declare module syntax as shown in 4455725. This preserves the "ambient-ness" of the ambient.d.ts.

Ensuring that dependencies are present with pnpm

This PR also fixes a possible footgun when importing modules with pnpm. By default, pnpm does not hoist dependencies to the top level of the node_modules. The direct dependencies of each package is thus locally scoped to that package only. If a package does not declare the dependency, pnpm will not resolve it.

This is problematic for the types/ of @sveltejs/enhanced-img because it imports from vite and svelte. Previously, these were declared as devDependencies, which causes pnpm to prune them from module resolution.

This PR fixes this by upgrading vite and svelte as direct dependencies. Alternatively, this may be downgraded to peerDependencies instead. Let me know if this is preferable.


Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

Copy link

changeset-bot bot commented Jun 17, 2024

🦋 Changeset detected

Latest commit: 5dbbe0b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/enhanced-img Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@benmccann benmccann requested a review from dummdidumm June 17, 2024 15:21
@BastiDood
Copy link
Contributor Author

@benmccann Heads up: CI lints are failing due to the outdated root lockfile. I have regenerated it using pnpm install. CI should pass now. Please don't mind the thousand-line diff. Thanks!

Top-level imports and exports automatically upgrade a TypeScript file to the module context. This is not desired in  because we want to keep this module ambient in order for the  to apply globally across all imports of .

By importing the  type inside the ambient module context, we preserve the ambient-ness of the  file.

https://www.typescriptlang.org/docs/handbook/modules/reference.html#ambient-modules

fix
@benmccann
Copy link
Member

I have regenerated it using pnpm install. CI should pass now. Please don't mind the thousand-line diff.

I didn't want the thousand line diff, so fixed it and did a force push. That means you won't be able to make changes on your branch without deleting it and checking it out again, but hopefully that's fine as there probably aren't further changes needed at this point

@BastiDood
Copy link
Contributor Author

I didn't want the thousand line diff, so fixed it and did a force push.

Thanks! This is super appreciated. The diffs look much cleaner now. 🚀

That means you won't be able to make changes on your branch without deleting it and checking it out again, but hopefully that's fine as there probably aren't further changes needed at this point.

No worries, nothing that a good ol' git fetch && git reset --hard origin/main can't fix. 😅

@BastiDood
Copy link
Contributor Author

Hello there! Just checking up and bumping this PR. It would be really nice to have this patch upstream so that our CI need not depend on patched/overridden versions. Thanks! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants