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.

Sign up for GitHub

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

Jump to bottom

fix(assets): Use uint8arrays instead of Buffer in code that can run outside of Node #9029

Merged
merged 4 commits into from
Nov 9, 2023
Merged

fix(assets): Use uint8arrays instead of Buffer in code that can run outside of Node #9029

merged 4 commits into from
Nov 9, 2023

Conversation

Princesseuh
Copy link
Member

@Princesseuh Princesseuh commented Nov 8, 2023 •
edited
Loading

Changes

Our service code can run outside of Node, so it shouldn't use Buffer, it should accept TypedArrays directly.

This is technically breaking, however:

  • It's extremely unlikely that someone relied on this being a Buffer when they made image services
  • As far as I know, no one has made actual full image services anyway
  • This corrects a flaw in the API, the way it was right now, you couldn't actually run things for real on non-Node hosts without making a service, a endpoint, spraying it with as any etc

It seems pretty safe to me, honestly. If we deem it not, I'm okay with delaying this to 4.0, no worries.

Fix withastro/adapters#54

Testing

Tests should still pass!

Docs

withastro/docs#5326

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Nov 8, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: c0b310f

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

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Nov 8, 2023
@Princesseuh Princesseuh marked this pull request as ready for review November 8, 2023 22:24
@github-actions github-actions bot added pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release labels Nov 8, 2023
github-actions[bot]
github-actions bot previously requested changes Nov 8, 2023
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

@alexanderniebuhr
Copy link
Member

!preview assets-uint8array

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 9, 2023

Snapshots have been released for the following packages:

  • astro@experimental--assets-uint8array
  • @astrojs/lit@experimental--assets-uint8array
  • @astrojs/markdown-remark@experimental--assets-uint8array
  • @astrojs/mdx@experimental--assets-uint8array
Publish Log 
🦋  warn ===============================IMPORTANT!===============================
🦋  warn Packages will be released under the experimental--assets-uint8array tag
🦋  warn ----------------------------------------------------------------------
🦋  info npm info astro
🦋  info npm info @astrojs/prism
🦋  info npm info @astrojs/rss
🦋  info npm info create-astro
🦋  info npm info @astrojs/alpinejs
🦋  info npm info @astrojs/lit
🦋  info npm info @astrojs/markdoc
🦋  info npm info @astrojs/mdx
🦋  info npm info @astrojs/node
🦋  info npm info @astrojs/partytown
🦋  info npm info @astrojs/preact
🦋  info npm info @astrojs/prefetch
🦋  info npm info @astrojs/react
🦋  info npm info @astrojs/sitemap
🦋  info npm info @astrojs/solid-js
🦋  info npm info @astrojs/svelte
🦋  info npm info @astrojs/tailwind
🦋  info npm info @astrojs/vercel
🦋  info npm info @astrojs/vue
🦋  info npm info @astrojs/internal-helpers
🦋  info npm info @astrojs/markdown-remark
🦋  info npm info @astrojs/telemetry
🦋  info npm info @astrojs/underscore-redirects
🦋  info astro is being published because our local version (0.0.0-assets-uint8array-20231109073138) has not been published on npm
🦋  warn @astrojs/prism is not being published because version 3.0.0 is already published on npm
🦋  warn @astrojs/rss is not being published because version 3.0.0 is already published on npm
🦋  warn create-astro is not being published because version 4.5.0 is already published on npm
🦋  warn @astrojs/alpinejs is not being published because version 0.3.1 is already published on npm
🦋  info @astrojs/lit is being published because our local version (0.0.0-assets-uint8array-20231109073138) has not been published on npm
🦋  warn @astrojs/markdoc is not being published because version 0.7.1 is already published on npm
🦋  info @astrojs/mdx is being published because our local version (0.0.0-assets-uint8array-20231109073138) has not been published on npm
🦋  warn @astrojs/node is not being published because version 6.0.3 is already published on npm
🦋  warn @astrojs/partytown is not being published because version 2.0.2 is already published on npm
🦋  warn @astrojs/preact is not being published because version 3.0.1 is already published on npm
🦋  warn @astrojs/prefetch is not being published because version 0.4.1 is already published on npm
🦋  warn @astrojs/react is not being published because version 3.0.4 is already published on npm
🦋  warn @astrojs/sitemap is not being published because version 3.0.3 is already published on npm
🦋  warn @astrojs/solid-js is not being published because version 3.0.2 is already published on npm
🦋  warn @astrojs/svelte is not being published because version 4.0.3 is already published on npm
🦋  warn @astrojs/tailwind is not being published because version 5.0.2 is already published on npm
🦋  warn @astrojs/vercel is not being published because version 5.2.0 is already published on npm
🦋  warn @astrojs/vue is not being published because version 3.0.4 is already published on npm
🦋  warn @astrojs/internal-helpers is not being published because version 0.2.1 is already published on npm
🦋  info @astrojs/markdown-remark is being published because our local version (0.0.0-assets-uint8array-20231109073138) has not been published on npm
🦋  warn @astrojs/telemetry is not being published because version 3.0.4 is already published on npm
🦋  warn @astrojs/underscore-redirects is not being published because version 0.3.3 is already published on npm
🦋  info Publishing "astro" at "0.0.0-assets-uint8array-20231109073138"
🦋  info Publishing "@astrojs/lit" at "0.0.0-assets-uint8array-20231109073138"
🦋  info Publishing "@astrojs/mdx" at "0.0.0-assets-uint8array-20231109073138"
🦋  info Publishing "@astrojs/markdown-remark" at "0.0.0-assets-uint8array-20231109073138"
🦋  success packages published successfully:
🦋  [email protected]
🦋  @astrojs/[email protected]
🦋  @astrojs/[email protected]
🦋  @astrojs/[email protected]
🦋  Creating git tags...
🦋  New tag:  [email protected]
🦋  New tag:  @astrojs/[email protected]
🦋  New tag:  @astrojs/[email protected]
🦋  New tag:  @astrojs/[email protected]
Build Log 

> [email protected] build /home/runner/work/astro/astro
> turbo run build --filter=astro --filter=create-astro --filter="@astrojs/*" --filter="@benchmark/*"

• Packages in scope: @astrojs/alpinejs, @astrojs/cloudflare, @astrojs/internal-helpers, @astrojs/lit, @astrojs/markdoc, @astrojs/markdown-remark, @astrojs/mdx, @astrojs/netlify, @astrojs/node, @astrojs/partytown, @astrojs/preact, @astrojs/prefetch, @astrojs/prism, @astrojs/react, @astrojs/rss, @astrojs/sitemap, @astrojs/solid-js, @astrojs/svelte, @astrojs/tailwind, @astrojs/telemetry, @astrojs/underscore-redirects, @astrojs/vercel, @astrojs/vue, @benchmark/timer, astro, create-astro
• Running build in 26 packages
• Remote caching enabled
::group::create-astro:build
cache hit, suppressing logs 895cd7a62cb990ee
::endgroup::
::group::@astrojs/internal-helpers:build
cache miss, executing 5189e4765f9441aa

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/internal-helpers
> astro-scripts build "src/**/*.ts" && tsc -p tsconfig.json

::endgroup::
::group::@astrojs/prism:build
cache miss, executing d2dcda1a046ff7cf

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/astro-prism
> astro-scripts build "src/**/*.ts" && tsc -p ./tsconfig.json

::endgroup::
::group::@astrojs/telemetry:build
cache miss, executing 470a5e5395d8d67a

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/telemetry
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/markdown-remark:build
cache miss, executing da9d79332a4a4061

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/markdown/remark
> astro-scripts build "src/**/*.ts" && tsc -p tsconfig.json

::endgroup::
::group::astro:build
cache miss, executing 293489ecd37ff434

> [email protected] build /home/runner/work/astro/astro/packages/astro
> pnpm run prebuild && astro-scripts build "src/**/*.{ts,js}" && tsc && pnpm run postbuild


> [email protected] prebuild /home/runner/work/astro/astro/packages/astro
> astro-scripts prebuild --to-string "src/runtime/server/astro-island.ts" "src/runtime/client/{idle,load,media,only,visible}.ts"


> [email protected] postbuild /home/runner/work/astro/astro/packages/astro
> astro-scripts copy "src/**/*.astro" && astro-scripts copy "src/**/*.wasm"

::endgroup::
::group::@astrojs/alpinejs:build
cache miss, executing 9a4c22834e330eca

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/integrations/alpinejs
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/lit:build
cache miss, executing e238051fad948f8f

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/integrations/lit
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@benchmark/timer:build
cache miss, executing 7bed3c4b0c9f2502

> @benchmark/[email protected] build /home/runner/work/astro/astro/benchmark/packages/timer
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/tailwind:build
cache miss, executing c59bfc9e6c5792e1

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/integrations/tailwind
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/react:build
cache miss, executing 46dccb75be5e82a8

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/integrations/react
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/preact:build
cache miss, executing 919c5285edb89ede

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/integrations/preact
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/solid-js:build
cache miss, executing 034eb7ea3b7bcaf5

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/integrations/solid
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/node:build
cache miss, executing 2a62520071a46632

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/integrations/node
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/vercel:build
cache miss, executing f3648f8fcd2c43fc

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/integrations/vercel
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/markdoc:build
cache miss, executing 9a704e8a904bb595

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/integrations/markdoc
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/prefetch:build
cache miss, executing 559c476c208b9a8a

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/integrations/prefetch
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/partytown:build
cache miss, executing 1f81329a4c5dba6d

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/integrations/partytown
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/svelte:build
cache miss, executing 5c7d1641861bdb54

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/integrations/svelte
> astro-scripts build "src/index.ts" && astro-scripts build "src/editor.cts" --force-cjs --no-clean-dist && tsc

::endgroup::
::group::@astrojs/rss:build
cache miss, executing 9fb0c309d3957904

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/astro-rss
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/underscore-redirects:build
cache miss, executing 21f3024ca82d0fc7

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/underscore-redirects
> astro-scripts build "src/**/*.ts" && tsc -p tsconfig.json

::endgroup::
::group::@astrojs/mdx:build
cache miss, executing db65bd47ae565b8f

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/integrations/mdx
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::
::group::@astrojs/vue:build
cache miss, executing 2ff02b40539d4065

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/integrations/vue
> astro-scripts build "src/index.ts" && astro-scripts build "src/editor.cts" --force-cjs --no-clean-dist && tsc

::endgroup::
::group::@astrojs/sitemap:build
cache miss, executing 95d6c9f5d98c7a6b

> @astrojs/[email protected] build /home/runner/work/astro/astro/packages/integrations/sitemap
> astro-scripts build "src/**/*.ts" && tsc

::endgroup::

 Tasks:    24 successful, 24 total
Cached:    1 cached, 24 total
  Time:    43.901s

alexanderniebuhr
alexanderniebuhr approved these changes Nov 9, 2023
View reviewed changes
Copy link
Member

@alexanderniebuhr alexanderniebuhr left a comment

Choose a reason for hiding this comment

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

I just tested this with a known good reproducible example and this fixes the issue, facing in non-node runtimes.

I also don't think this is breaking, so I would be very happy to get this out sooner than later.

/lgtm

ematipico
ematipico approved these changes Nov 9, 2023
View reviewed changes
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

The changes look good to me. I believe this PR should be a patch because we used the wrong types (data structures) to handle binary data. For users affected by this wrong assumption from our end, we can provide a migration path to patch their code.

@Princesseuh Princesseuh mentioned this pull request Nov 9, 2023
Merged
@Princesseuh Princesseuh merged commit 29b83e9 into main Nov 9, 2023
4 checks passed
@Princesseuh Princesseuh deleted the fix/uint8array-assets branch November 9, 2023 13:05
@astrobot-houston astrobot-houston mentioned this pull request Nov 9, 2023
Merged
natemoo-re pushed a commit that referenced this pull request Nov 22, 2023
@Princesseuh @natemoo-re
fix(assets): Use uint8arrays instead of Buffer in code that can run o…
50a6f66 
…utside of Node (#9029)

* fix(assets): Use uint8arrays instead of Buffer in code that can run outside of Node

* chore: changeset

* docs: update changeset with more information on what to do if a Buffer is important

* nit: do a patch instead
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexanderniebuhr alexanderniebuhr alexanderniebuhr approved these changes

@ematipico ematipico ematipico approved these changes

@github-actions github-actions[bot] github-actions[bot] left review comments

Assignees
No one assigned
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

astro:assets noop does not work on Cloudflare Runtime
3 participants
@Princesseuh @alexanderniebuhr @ematipico