benmccann Github contribution chart
benmccann Github Stats
benmccann Most Used Languages

Activity

29 Sep 2022

Merge

Benmccann

Added Option to Automatically Add Vitest to New Projects using svelte-create

Issue

Getting started with unit testing in Sveltekit can be confusing, which was brought up in this issue here https://github.com/sveltejs/kit/issues/4423

Resolution

  • Added the ability to automatically add Vitest with example tests to new projects using the svelte-create package. This should help people new to unit tests in Sveltekit get started with reasonable defaults by scaffolding the configuration for them, and giving them example tests to start out with.

Testing

  • Manually tested this with every config to make sure it worked.

Possible problems

  • I have not tested this Vitest config when emulating Sveltekit specific behavior such as $app/env and $app/stores I might need input from @benmccann on this one...

Forked On 29 Sep 2022 at 04:24:49

Benmccann

SvelteKit uses `.spec` rather than `.test` for its own unit tests, so I wonder if we should do the same here
On 29 Sep 2022 at 04:24:49

Benmccann

Added Option to Automatically Add Vitest to New Projects using svelte-create

Issue

Getting started with unit testing in Sveltekit can be confusing, which was brought up in this issue here https://github.com/sveltejs/kit/issues/4423

Resolution

  • Added the ability to automatically add Vitest with example tests to new projects using the svelte-create package. This should help people new to unit tests in Sveltekit get started with reasonable defaults by scaffolding the configuration for them, and giving them example tests to start out with.

Testing

  • Manually tested this with every config to make sure it worked.

Possible problems

  • I have not tested this Vitest config when emulating Sveltekit specific behavior such as $app/env and $app/stores I might need input from @benmccann on this one...

Merged On 29 Sep 2022 at 04:24:50

Benmccann

Commented On 29 Sep 2022 at 04:24:50
Issue Comment

Benmccann

building issue in production with some NPM packages

Describe the bug

We are trying to migrate our sveltekit from 1.0.0-next.355 to the latest version - 1.0.0-next.504, and adapter-node from 1.0.0-next.78 to 1.0.0-next.95.

yarn run dev and yarn run build -> yarn run preview are successfully done, but when I launch server by nodejs through node build/index.js.

I asked this issue in discord, I got few responses. Someone said there is similar issue when he migrated from esbuild to rollup, and adapter-node also migrated from esbuild since v93.

I think our source code migration is done correctly according to the sveltekit guideline, but I guess there is some building issue by adapter-node or vite with some of NPM packages since adapter-node changed from esbuild to rollup.

Reproduction

I found there are at least two endpoints in our project having building issue in sveltekit. I created a very minimal project with the latest sveltekit version.

Please follow the reproduction procedure in README.md at the following repository,

https://github.com/UNDP-Data/sveltekit-build-test

Furthermore, I created another branch in the repo which has downgraded sveltekit.

https://github.com/UNDP-Data/sveltekit-build-test/tree/downgrade-sveltekit

You can compare both main branch and downgrade-sveltekit branch how the server works in production by using node build

Logs

In our project, the following logs were produced by using `yarn run build -> node build` commands.


ReferenceError: require is not defined in ES module scope, you can use import instead
This file is being treated as an ES module because it has a '.js' file extension and '/Users/j_igarashi/Documents/git/UNDP-Data/geohub/package.json' contains "type": "module". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.
    at file:///Users/j_igarashi/Documents/git/UNDP-Data/geohub/build/server/chunks/variables-e4a6492e.js:126157:25
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:527:24)
    at async Promise.all (index 1)
    at async render_page (file:///Users/j_igarashi/Documents/git/UNDP-Data/geohub/build/server/index.js:2297:19)
    at async resolve (file:///Users/j_igarashi/Documents/git/UNDP-Data/geohub/build/server/index.js:2787:22)
    at async respond (file:///Users/j_igarashi/Documents/git/UNDP-Data/geohub/build/server/index.js:2825:22)
    at async Array.ssr (file:///Users/j_igarashi/Documents/git/UNDP-Data/geohub/build/handler.js:19060:3) 

The following log is when I downgraded adapter-node to 1.0.0-next.92.

> Using @sveltejs/adapter-node
✘ [ERROR] Could not resolve "pg-native"

    node_modules/pg/lib/native/client.js:4:21:
      4 │ var Native = require('pg-native')
        ╵                      ~~~~~~~~~~~

  You can mark the path "pg-native" as external to exclude it from the bundle, which will remove
  this error. You can also surround this "require" call with a try/catch block to handle this
  failure at run-time instead of bundle-time.

✘ [ERROR] No loader is configured for ".node" files: node_modules/canvas/build/Release/canvas.node

    node_modules/canvas/lib/bindings.js:3:25:
      3 │ const bindings = require('../build/Release/canvas.node')
        ╵                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error during build:
Error: Build failed with 2 errors:
node_modules/canvas/lib/bindings.js:3:25: ERROR: No loader is configured for ".node" files: node_modules/canvas/build/Release/canvas.node
node_modules/pg/lib/native/client.js:4:21: ERROR: Could not resolve "pg-native"
    at failureErrorWithLog 

Other logs for our testing source code are in UNDP-Data/sveltekit-build-test repo's README.

System Info

Our server is running in Azure AppService. The following log was generated in our repository in my laptop.

 System:
    OS: macOS 12.6
    CPU: (8) arm64 Apple M1
    Memory: 107.97 MB / 16.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 18.9.0 - ~/.nvm/versions/node/v18.9.0/bin/node
    Yarn: 1.22.11 - /opt/homebrew/bin/yarn
    npm: 8.19.1 - ~/.nvm/versions/node/v18.9.0/bin/npm
  Browsers:
    Chrome: 106.0.5249.61
    Firefox: 92.0
    Safari: 16.0
  npmPackages:
    @sveltejs/adapter-node: ^1.0.0-next.78 => 1.0.0-next.96 
    @sveltejs/kit: ^1.0.0-next.355 => 1.0.0-next.504 
    svelte: ^3.48.0 => 3.50.1 

The following log is for our testing repository UNDP-Data/sveltekit-build-test. Also, it was generated in my laptop.

 System:
    OS: macOS 12.6
    CPU: (8) arm64 Apple M1
    Memory: 86.81 MB / 16.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 18.9.0 - ~/.nvm/versions/node/v18.9.0/bin/node
    Yarn: 1.22.11 - /opt/homebrew/bin/yarn
    npm: 8.19.1 - ~/.nvm/versions/node/v18.9.0/bin/npm
  Browsers:
    Chrome: 106.0.5249.61
    Firefox: 92.0
    Safari: 16.0
  npmPackages:
    @sveltejs/adapter-node: ^1.0.0-next.96 => 1.0.0-next.96 
    @sveltejs/kit: ^1.0.0-next.505 => 1.0.0-next.505 
    svelte: ^3.44.0 => 3.50.1 
    vite: ^3.1.0 => 3.1.4 

Severity

blocking an upgrade

Additional Information

Yesterday, I posted the same one in discussion (https://github.com/sveltejs/kit/discussions/7069).

Forked On 29 Sep 2022 at 03:43:35

Benmccann

Yes, bundling everything should work. I believe the primary error from running in an unbundled fashion is coming from https://github.com/salesforce/tough-cookie/issues/256

I'm going to close this in favor of that issue since the issue is not in SvelteKit itself

Commented On 29 Sep 2022 at 03:43:35
Issue Comment

Benmccann

Prerender fails when the route contains Non-ASCII Characters

Describe the bug

Prerendering process fails during setting the response header of x-sveltekit-routeid when routeId contains Non-ASCII Characters. https://github.com/sveltejs/kit/blob/master/packages/kit/src/runtime/server/index.js#L301

TypeError: Cannot convert argument to a ByteString because the character atindex 0 has a value of 21021 which is greater than 255. 

Reproduction

https://github.com/kwchang0831/issue-sveltekit-prerender-unicode-path

To Reproduce:

  1. Create a route with Non-ASCII Characters. eg: src/routes/初めまして/+page.svelte.
  2. Set export const prerender = true; in +layout.ts.
  3. Run pnpm build

Logs

No response

System Info

System:
    OS: Linux 5.15 Ubuntu 22.04.1 LTS 22.04.1 LTS (Jammy Jellyfish)
    CPU: (12) x64 Intel(R) Xeon(R) CPU D-1541 @ 2.10GHz
    Memory: 21.55 GB / 31.35 GB
    Container: Yes
    Shell: 3.5.1 - /usr/bin/fish
  Binaries:
    Node: 18.7.0 - ~/.asdf/installs/nodejs/18.7.0/bin/node
    Yarn: 1.22.19 - ~/.asdf/shims/yarn
    npm: 8.15.0 - ~/.asdf/plugins/nodejs/shims/npm
  Browsers:
    Firefox: 105.0.1
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.80 
    @sveltejs/kit: next => 1.0.0-next.505 
    svelte: ^3.44.0 => 3.50.1 
    vite: ^3.1.0 => 3.1.4 

Severity

serious, but I can work around it

Additional Information

What I have tried to solve the issue:

Encode the routeId

packages/kit/src/runtime/server/index.js#301

if (state.prerendering && event.routeId !== null) {
  response.headers.set('x-sveltekit-routeid', encodeURI(event.routeId));
} 

packages/kit/src/core/prerender/prerender.js#L423

for (const [route_id, prerender] of prerender_map) {
  if (prerender === true && !prerendered_routes.has(encodeURI(route_id))) {
    not_prerendered.push(route_id);
  }
} 

I am not sure if this is correct way to do.

Also, while I was trying to figure out the issue, I found that the routing will fail if the route is already encoded. eg: src/routes/%E5%88%9D%E3%82%81/+page.svelte. I guess I should probably sumbit another issue for that.

Forked On 29 Sep 2022 at 03:08:32

Benmccann

We had to do encodeURI for the link header: https://github.com/sveltejs/kit/blob/a4e5814d2f6cd7e6700076df9ea1f99d4161a873/packages/kit/src/runtime/server/page/render.js#L248

I'm guessing we need to do the same here

Commented On 29 Sep 2022 at 03:08:32
Issue Comment

Benmccann

Access route result in Not Found during dev when the route is encoded URI

Describe the bug

Routing result is Not Found if the route is encoded URI during dev server. eg: With route src/routes/%E5%88%9D%E3%82%81/+page.svelte when navigating to http://localhost:5173/%E5%88%9D%E3%82%81

Not found: /src/routes/%E5%88%9D%E3%82%81/+page.svelte
Error: Not found: /src/routes/%E5%88%9D%E3%82%81/+page.svelte 

However, preview works as expected. After pnpm build && pnpm preview, the route http://localhost:4173/%E5%88%9D%E3%82%81 is accessible.

For the reference:

encodeURI('初め') => '%E5%88%9D%E3%82%81'

Reproduction

https://github.com/kwchang0831/issue-sveltekit-already-encoded-route

To Reproduce:

  1. Create a route with encoded URI. eg: src/routes/%E5%88%9D%E3%82%81/+page.svelte.
  2. Set export const prerender = true; in +layout.ts.
  3. Run pnpm dev.
  4. In browser, navigate to the route http://localhost:5173/%E5%88%9D%E3%82%81.

Logs

No response

System Info

System:
    OS: Linux 5.15 Ubuntu 22.04.1 LTS 22.04.1 LTS (Jammy Jellyfish)
    CPU: (12) x64 Intel(R) Xeon(R) CPU D-1541 @ 2.10GHz
    Memory: 22.60 GB / 31.35 GB
    Container: Yes
    Shell: 3.5.1 - /usr/bin/fish
  Binaries:
    Node: 18.7.0 - ~/.asdf/installs/nodejs/18.7.0/bin/node
    Yarn: 1.22.19 - ~/.asdf/shims/yarn
    npm: 8.15.0 - ~/.asdf/plugins/nodejs/shims/npm
  Browsers:
    Firefox: 105.0.1
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.80 
    @sveltejs/kit: next => 1.0.0-next.505 
    svelte: ^3.44.0 => 3.50.1 
    vite: ^3.1.0 => 3.1.4 

Severity

serious, but I can work around it

Additional Information

No response

Forked On 29 Sep 2022 at 02:09:50

Benmccann

Name your folder 初め instead of %E5%88%9D%E3%82%81 and I expect it will work

Commented On 29 Sep 2022 at 02:09:50

Benmccann

fix: let vite create final paths

  • Quick Checklist
  • [x] I have read the contributing guidelines
  • [n/a] I have written new tests, as applicable (for bug fixes / features)
  • [n/a] Docs have been added / updated (for bug fixes / features)
  • [x] I have added a changeset, if applicable
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...) This PR fixes the paths of images in the Vite build result. Before the filename was added to viteConfig.base which leads to relative urls that don't work anymore in nested paths.

  • What is the new behavior (if this is a feature change)? Instead of inserting the final path in imagetools, the existing placeholder logic from Vite is used and Vite will do the replacement.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

  • Other information:

Forked On 29 Sep 2022 at 02:03:55

Benmccann

a code comment here would be nice. e.g. it'd be helpful to know what the constructed value is supposed to look like. i'm also curious if there's a section of Vite's code that this is mirroring that we could link to
On 29 Sep 2022 at 02:03:55

Benmccann

fix: let vite create final paths

  • Quick Checklist
  • [x] I have read the contributing guidelines
  • [n/a] I have written new tests, as applicable (for bug fixes / features)
  • [n/a] Docs have been added / updated (for bug fixes / features)
  • [x] I have added a changeset, if applicable
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...) This PR fixes the paths of images in the Vite build result. Before the filename was added to viteConfig.base which leads to relative urls that don't work anymore in nested paths.

  • What is the new behavior (if this is a feature change)? Instead of inserting the final path in imagetools, the existing placeholder logic from Vite is used and Vite will do the replacement.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

  • Other information:

Merged On 29 Sep 2022 at 02:03:56

Benmccann

Commented On 29 Sep 2022 at 02:03:56

Benmccann

feat: Support for preprocessors in .stories.svelte

Fixes #4

Uses the preprocess settings from the svelteOptions defined in .storybook/main.js The same preprocess settings that are used for regular .svelte files are now also applied to the *.stories.svelte files

Merged On 29 Sep 2022 at 01:59:32

Benmccann

I'm totally fine with dropping support for Jest 26. On the other hand, I'm quite concerned that this is not using svelte.config.js. We absolutely should not be making users manually specify all their options again a second time via svelteOptions.

Commented On 29 Sep 2022 at 01:59:32
Merge

Benmccann

[feat] implement optional route params

Closes #5072

I chose the [[optional]] syntax for alignment with the wider ecosystem/how other frameworks do it. Probably needs some more tests, also wondering if we should have a check that tells you that you have both a required and optional param matching the same, which is forbidden. Apart from that it's ... done? I don't know. Feels like I'm missing something, but my local playground tests succeeded.

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

  • [x] 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
  • [x] This message body should clearly illustrate what problems it solves.
  • [x] 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

  • [x] 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. All changesets should be patch until SvelteKit 1.0

Forked On 28 Sep 2022 at 10:30:59

Benmccann

```suggestion A route like `[lang]/home` contains a parameter named `lang` which is required. Sometimes it's beneficial to make these parameters optional, so that in this example both `home` and `en/home` point to the same page. You can do that by wrapping the parameter in another bracket pair: `[[lang]]/home` ```
On 28 Sep 2022 at 10:30:59

Benmccann

[feat] implement optional route params

Closes #5072

I chose the [[optional]] syntax for alignment with the wider ecosystem/how other frameworks do it. Probably needs some more tests, also wondering if we should have a check that tells you that you have both a required and optional param matching the same, which is forbidden. Apart from that it's ... done? I don't know. Feels like I'm missing something, but my local playground tests succeeded.

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

  • [x] 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
  • [x] This message body should clearly illustrate what problems it solves.
  • [x] 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

  • [x] 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. All changesets should be patch until SvelteKit 1.0

Merged On 28 Sep 2022 at 10:31:00

Benmccann

Commented On 28 Sep 2022 at 10:31:00
Issue Comment

Benmccann

fix: let vite create final paths

  • Quick Checklist
  • [x] I have read the contributing guidelines
  • [n/a] I have written new tests, as applicable (for bug fixes / features)
  • [n/a] Docs have been added / updated (for bug fixes / features)
  • [x] I have added a changeset, if applicable
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...) This PR fixes the paths of images in the Vite build result. Before the filename was added to viteConfig.base which leads to relative urls that don't work anymore in nested paths.

  • What is the new behavior (if this is a feature change)? Instead of inserting the final path in imagetools, the existing placeholder logic from Vite is used and Vite will do the replacement.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

  • Other information:

Forked On 28 Sep 2022 at 09:10:09

Benmccann

Could you please add a test?

Commented On 28 Sep 2022 at 09:10:09
Issue Comment

Benmccann

Bug: Paths are relative instead of absolute

I've just upgraded a project from sveltekit 3xx to 4xx. The formerly absolute paths created by imagetools are now relative (to /) and therefore not working on any subpage.

<script>
	import Img from './image.jpg?webp&meta'
</script>

{Img.src} 

will result in

./_app/immutable/assets/image-a774634e.webp 

In dev mode everything works, but as soon as I build the path is wrong. Before the upgrade the result was /_app/immutable/assets/image-a774634e.webp for {Img.src}. So without the leading ..

Repro

  • clone https://github.com/Amerlander/image-import-test and npm i, npm run build:
  • Open /about and check the sources below the headline.
  • Theres also a import without using imagetools which still produces the right path

Forked On 28 Sep 2022 at 09:08:34

Benmccann

This may be from imagetools after all. Can you check if https://github.com/JonasKruckenberg/imagetools/pull/400 would fix the issue you're facing?

Commented On 28 Sep 2022 at 09:08:34
Issue Comment

Benmccann

vite-imagetools doesn't pick up my images on build (SvelteKit + MDsveX)

I was using MDsveX to render a blog page but the images I import into it don't work when transformed into webp using imagetools.

My code on the markdown page

import thumbnail from '$lib/assets/images/project/reza/reza-thumbnail.png?webp'; 

This is how it looks on dev

photo_2022-09-20_11-58-00

This is the same page on preview

photo_2022-09-20_12-08-29

Forked On 28 Sep 2022 at 09:08:00

Benmccann

Can you check if https://github.com/JonasKruckenberg/imagetools/pull/400 would fix the issue you're facing?

Commented On 28 Sep 2022 at 09:08:00
Issue Comment

Benmccann

Images don't show in dev mode with Laravel backend

If you try this plugin with a Laravel + Vite + Vue stack, all the images will throw a 404 (Not Found) error and not show on the page.

This is because the tool is returning relative paths and normally an asset URL imported through Vite would have an absolute URL, and in the context of a backend such as Laravel the lack of absolute URL breaks the images.

Assuming you're running in dev mode Without imagetools loader ( normal image importing ):

import Cat from '@/images/cat.png'

console.log(Cat)
// Output: http://localhost:3000/resources/images/cat.png 

With imagetools loader:

import Cat from '@/images/cat.png?width=200'

console.log(Cat)
// Output: /@imagetools/5812d5ee51f96cb9c239acca9f2ecc22bd83998a
// Throws a not found error. 

Note that build works fine. It's only a dev mode issue.

Forked On 28 Sep 2022 at 09:06:11

Benmccann

Can you check if https://github.com/JonasKruckenberg/imagetools/pull/400 would fix the issue you're facing?

Commented On 28 Sep 2022 at 09:06:11
Issue Comment

Benmccann

[Fix] broken assets path

Fixes #6326

This fixes broken usage of imagetools and possibly other vite plugins as well - when asset paths are used with the static adapter. The breaking PR was #4250.

This fix introduces a new config option ~~rootRelativeAssets~~ relativeAssets, which defaults to ~~true~~ false, restoring the way things worked prior to the breaking PR, => namely: non-relative asset paths (or a "root-based asset path").

EDIT: Changed option name and logic

This approach may or may not be the correct one and may be up for discussion or be rejected completely. For that reason, I have not written a test for that, yet. But I have tested the fix successfully on a project which suffered that issue.

If this is a viable fix, I'll gladly complete this PR with anything else needed.

...and thank you all for this amazing project :1st_place_medal:


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

  • [x] 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
  • [x] This message body should clearly illustrate what problems it solves.
  • [ ] Ideally, include a test that fails without this PR but passes with it.

Tests

  • [x] 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. All changesets should be patch until SvelteKit 1.0

Forked On 28 Sep 2022 at 09:05:57

Benmccann

Can you check if https://github.com/JonasKruckenberg/imagetools/pull/400 would fix the issue you're facing?

Commented On 28 Sep 2022 at 09:05:57

Benmccann

fix: add an exports map

Created On 28 Sep 2022 at 08:52:50
Create Branch
Benmccann In benmccann/imagetools Create Branchexports

Benmccann

Load and transform images using a toolbox :toolbox: of custom import directives!

On 28 Sep 2022 at 08:49:14

Benmccann

tests

Pushed On 28 Sep 2022 at 08:27:33

Benmccann

fix: process URLs with no query string

Pushed On 28 Sep 2022 at 08:06:42

Benmccann

fix: process URLs with no query string

Created On 28 Sep 2022 at 07:56:15

Benmccann

fix: process URLs with no query string

Pushed On 28 Sep 2022 at 07:55:34
Create Branch
Benmccann In benmccann/imagetools Create Branchno-query-param

Benmccann

Load and transform images using a toolbox :toolbox: of custom import directives!

On 28 Sep 2022 at 07:53:57
Issue Comment

Benmccann

Question: Is it possible to make sure the image output is always webp?

I want to make all of the images on my website webp. I don't want to constantly type ?webp' behind the imports.

Forked On 28 Sep 2022 at 06:39:20

Benmccann

I don't know why i need to add ?&imagetools but otherwise it does not work, i hope it helps you.

Yeah, that seems like it's potentially a bug. It's coming from this line:

https://github.com/JonasKruckenberg/imagetools/blob/a5d1fb92b915e6b38925b363203aeed324d709cb/packages/vite/src/index.ts#L21

Commented On 28 Sep 2022 at 06:39:20

Benmccann

[Fix] broken assets path

Fixes #6326

This fixes broken usage of imagetools and possibly other vite plugins as well - when asset paths are used with the static adapter. The breaking PR was #4250.

This fix introduces a new config option ~~rootRelativeAssets~~ relativeAssets, which defaults to ~~true~~ false, restoring the way things worked prior to the breaking PR, => namely: non-relative asset paths (or a "root-based asset path").

EDIT: Changed option name and logic

This approach may or may not be the correct one and may be up for discussion or be rejected completely. For that reason, I have not written a test for that, yet. But I have tested the fix successfully on a project which suffered that issue.

If this is a viable fix, I'll gladly complete this PR with anything else needed.

...and thank you all for this amazing project :1st_place_medal:


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

  • [x] 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
  • [x] This message body should clearly illustrate what problems it solves.
  • [ ] Ideally, include a test that fails without this PR but passes with it.

Tests

  • [x] 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. All changesets should be patch until SvelteKit 1.0

Merged On 28 Sep 2022 at 06:32:34

Benmccann

this doesn't seem like the right fix to me for a couple reasons

one is that we should make it work with relative assets

the other is that this seems to also be affecting laravel, so it's quite possibly an issue in vite-imagetools: https://github.com/JonasKruckenberg/imagetools/issues/396

Commented On 28 Sep 2022 at 06:32:34
Issue Comment

Benmccann

Calling imagetools from img src?

Hi

Anyone knows if there is a way to call imagetools transformation directly from the html? Like <img src="./image.jpg?width=300;500;700&format=webp;avif;png" />

cheers

Forked On 28 Sep 2022 at 06:28:01

Benmccann

For Svelte projects you can use https://github.com/bluwy/svelte-preprocess-import-assets

Commented On 28 Sep 2022 at 06:28:01
Issue Comment

Benmccann

Allow output format directives in defaultDirectives

Hello 👋

My collaborator and I are trying to use vite-imagetools to transition a project from the Gatsby ecosystem. We weren't able to use a meta format directive from within the defaultDirectives override, as the actual import's parameters are checked for output formats.

I assume allowing this would be as simple as changing srcURL.searchParams.has(key) to directives.has(key) here:

https://github.com/JonasKruckenberg/imagetools/blob/58eabc941d5270f65e353f4b3466f4d337a109eb/packages/vite/src/index.ts#L97-L106

Is there a reason this isn't permitted, or is this an oversight? Thanks for any info!

(And, before you ask, yes, I can think of ways in which our use of defaultDirectives in some cases rubs up against the spirit of some of the comments in #160, but we aren't able to make use of query parameters with dynamic import vars without hacking the rollup plugin, and I feel fine using defaultDirectives in this spot.)

Forked On 28 Sep 2022 at 06:25:58

Benmccann

This has been done:

https://github.com/JonasKruckenberg/imagetools/blob/a5d1fb92b915e6b38925b363203aeed324d709cb/packages/vite/src/index.ts#L99

Commented On 28 Sep 2022 at 06:25:58