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

imgbundler and CLI fixes #278

Merged
merged 10 commits into from
Nov 30, 2022
Merged

imgbundler and CLI fixes #278

merged 10 commits into from
Nov 30, 2022

Conversation

nhooyr
Copy link
Contributor

@nhooyr nhooyr commented Nov 30, 2022

See individual commits.

And please test @alixander

@nhooyr nhooyr requested review from alixander and a team November 30, 2022 10:44
@nhooyr
Copy link
Contributor Author

nhooyr commented Nov 30, 2022

Example:

script

server: {
  shape: image
  icon: https://iconss.terrastruct.com/tech/022-server.svg
}

github: {
  shape: image
  icon: https://icon2.terrastruct.com/dev/github.svg
}

tri: {
  shape: image
  icon: /tmp/jingle.png
}

server -> github

watch terminal

image

watch render

Screenshot 2022-11-30 at 3 08 58 AM

prod terminal

Screenshot 2022-11-30 at 3 11 11 AM

prod render

Screenshot 2022-11-30 at 3 11 32 AM

thoughts

not a fan of the prod render, maybe we ought to include the error within the svg just like I was doing before.
Or at the very least replace the image with a placeholder.

@nhooyr
Copy link
Contributor Author

nhooyr commented Nov 30, 2022

I opened #280

Copy link
Collaborator

@alixander alixander left a comment

Choose a reason for hiding this comment

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

👏 . imgbundler code so much cleaner.

add in the release notes that the bundle flag now works.

removing the conversion from text/xml to image/svg+xml would cause issues viewing in browser i think. it's why i did it in the first place: https://stackoverflow.com/questions/31755822/why-does-this-svg-work-being-viewed-raw-in-the-browser-but-not-in-a-webpage

cmd/d2/main.go Show resolved Hide resolved
cmd/d2/main.go Outdated Show resolved Hide resolved
cmd/d2/watch.go Outdated Show resolved Hide resolved
cmd/d2/watch.go Outdated Show resolved Hide resolved
lib/imgbundler/imgbundler.go Show resolved Hide resolved
lib/imgbundler/imgbundler.go Show resolved Hide resolved
lib/imgbundler/imgbundler.go Outdated Show resolved Hide resolved
@alixander
Copy link
Collaborator

alixander commented Nov 30, 2022

for me, it doesn't show the broken img. it used to

Screen Shot 2022-11-30 at 10 17 08 AM

@nhooyr
Copy link
Contributor Author

nhooyr commented Nov 30, 2022

@alixander what's your d2 text there?

@alixander
Copy link
Collaborator

test: {
  icon: https://no.com
}

test2: {
  icon: https://no.com
}

works: {
  icon: https://www.howtogeek.com/wp-content/uploads/2018/06/shutterstock_1006988770.png
}

test -> test2

@nhooyr
Copy link
Contributor Author

nhooyr commented Nov 30, 2022

Weird I can confirm, don't see the image either. I don't know why in that particular example. Mine still works as expected.

Screenshot 2022-11-30 at 2 48 55 PM

Could it be unrelated to this PR? I'm not sure how any of my changes could have caused this.

@alixander
Copy link
Collaborator

yeah, maybe some SVG thing. we'll just note in that issue.

cc @alixander this has been broken for so long lol
From when the code was in the monorepo.
- Make bundle flag work
- Display error and update render at the same time in watch mode.
  Before we would just display the render and not show the error.

- Rename imgbundler.InlineX functions to BundleX
- Print imgbundler fetch/readFile errors as they happen in the workers
  instead of coalescing and printing at the end.
- Minor performance improvements by using []byte everywhere possible.
- Improved symbol naming in imgbundler code
- **major**: Ignore already bundled images instead of trying to os.ReadFile them.
The order of priority in detecting the mime type is now:
- Content-Type response header
- File path extension
- http.DetectContentType
@nhooyr nhooyr merged commit 29562fe into terrastruct:master Nov 30, 2022
@nhooyr nhooyr deleted the fixes-d6f3 branch November 30, 2022 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants