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

chore(web): Fix .d.ts overwrite build issue #10431

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

Tobbe
Copy link
Member

@Tobbe Tobbe commented Apr 8, 2024

A clean build would work, but trying to build again after changing something in @redwoodjs/web you'd get errors like these

image

After a long session with git bisect I narrowed it down to the commit that introduced this regression. It was #10412

Specifically it's this line https://github.com/redwoodjs/redwood/pull/10412/files#diff-3a6b1602e214ff07d5aea85aead0ff0e5c99e225f658b476050cc3f730b8a8e7R5 import type { TagDescriptor } from '@redwoodjs/web'. That pulls in all the type def files for @redwoodjs/web and makes them input files.

I tried with a relative import instead (./src/components/htmlTags), but got the same error.
Tried a script instead of a module. Same error.
Tried putting it inside ./src/. Same error.

And when it finally did pick up on the types correctly, it revealed another type issue we had with the return value from extractFromAssetMap where TS couldn't figure out if it was returning the css (string[]) or the meta tags (TagDescriptor[]), so it gave us both (string[] | TagDescriptor[]) which you're not really allowed to pass to isTitleTag for example. So I had to fix that issue too.

__REDWOOD__ASSET_MAP is only used in a way where types matter in this one file, so I decided to make the window type augmentation local to the module instead of global. The other place we use __REDWOOD__ASSET_MAP is in a string that we inject in user's apps during streaming. There's no type checking/safety going on there, so a global type definition is not needed.

@Tobbe Tobbe added the changesets-ok Override the changesets check label Apr 8, 2024
@Tobbe Tobbe added this to the chore milestone Apr 8, 2024
@Tobbe Tobbe added the release:chore This PR is a chore (means nothing for users) label Apr 8, 2024
@Tobbe Tobbe merged commit b379670 into redwoodjs:main Apr 8, 2024
49 of 56 checks passed
@Tobbe Tobbe deleted the tobbe-web-fix-d-ts-overwrite branch April 8, 2024 10:52
dac09 added a commit to dac09/redwood that referenced this pull request Apr 9, 2024
…auth-provider-p1

* 'main' of github.com:redwoodjs/redwood:
  fix(middleware): Handle POST requests in middleware router too (redwoodjs#10418)
  chore(ci): get ci running on next (redwoodjs#10432)
  RSC: Explain noExternal vite config option (redwoodjs#10429)
  chore(web): Fix .d.ts overwrite build issue (redwoodjs#10431)
  chore(web): .js imports to prep for ESM (redwoodjs#10430)
  chore(refactor): Split rwjs/forms up into several smaller logical units (redwoodjs#10428)
  chore(rsc): simplify `noExternals` config (redwoodjs#10220)
  chore(deps): Update vite to 5.2.8 (redwoodjs#10427)
  chore(auth): Convert `@redwoodjs/auth` to ESM+CJS dual build (redwoodjs#10417)
  chore(framework-tools): Warn about missing metafile (redwoodjs#10426)
  chore(test): Switch rwjs/auth over to vitest (redwoodjs#10423)
  chore(whatwg-fetch): Switch to importing instead of requiring (redwoodjs#10424)
  chore(deps): bump undici from 5.28.3 to 5.28.4 in /.github/actions/check_changesets (redwoodjs#10421)
  chore(route-manifest): Add relativeFilePath to route manifest (redwoodjs#10416)
dac09 added a commit that referenced this pull request Apr 11, 2024
…th-mw-auth

* 'main' of github.com:redwoodjs/redwood: (21 commits)
  fix(auth): Handle when authorization header is lowercased (#10442)
  Update rbac.md - code match (#10405)
  chore: make crwa e2e test work across branches (#10437)
  feat: [Auth] Common AuthProvider & use* changes for middleware auth (#10420)
  fix(cli): only show webpack options for dev if `bundler = "webpack"` (#10359)
  fix(vercel): specify build env vars as a string (#10436)
  fix(vercel): write `vercel.json` as a part of setup (#10355)
  fix(middleware): Handle POST requests in middleware router too (#10418)
  chore(ci): get ci running on next (#10432)
  RSC: Explain noExternal vite config option (#10429)
  chore(web): Fix .d.ts overwrite build issue (#10431)
  chore(web): .js imports to prep for ESM (#10430)
  chore(refactor): Split rwjs/forms up into several smaller logical units (#10428)
  chore(rsc): simplify `noExternals` config (#10220)
  chore(deps): Update vite to 5.2.8 (#10427)
  chore(auth): Convert `@redwoodjs/auth` to ESM+CJS dual build (#10417)
  chore(framework-tools): Warn about missing metafile (#10426)
  chore(test): Switch rwjs/auth over to vitest (#10423)
  chore(whatwg-fetch): Switch to importing instead of requiring (#10424)
  chore(deps): bump undici from 5.28.3 to 5.28.4 in /.github/actions/check_changesets (#10421)
  ...
@Josh-Walker-GM
Copy link
Collaborator

Switching this to SSR milestone as it appears to build off of SSR PRs - namely #8978 and #10412 as far as I can tell.

@Josh-Walker-GM Josh-Walker-GM modified the milestones: chore, SSR Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changesets-ok Override the changesets check release:chore This PR is a chore (means nothing for users)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants