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

feat: add new resolve web asset function #2334

Merged
merged 5 commits into from
Jul 19, 2024
Merged

Conversation

bohdanprog
Copy link
Member

Summary

Add a new function to resolve Image asset uri.

Compatibility

OS Implemented
Web

@bohdanprog bohdanprog requested a review from WoLewicki July 7, 2024 10:07
@bohdanprog bohdanprog self-assigned this Jul 8, 2024
Copy link
Member

@WoLewicki WoLewicki left a comment

Choose a reason for hiding this comment

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

Left some comments

index.d.ts Outdated
@@ -0,0 +1 @@
declare module '@react-native/assets-registry/registry';
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? Should it be also added to files that end up in release?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, without it, we have a problem with typescript

Copy link
Member

Choose a reason for hiding this comment

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

You answered only the first question here 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

@WoLewicki Ah, you're right sorry. Hm... I'm a little confused because I'm not sure how we build the library 🧐

Copy link
Member

Choose a reason for hiding this comment

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

You can run npm pack to get the package that would be available on npm. Then you can add this package to a new project to test it locally. Is it what you meant?

Copy link
Member

Choose a reason for hiding this comment

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

Does this index.d.ts file end up in the package then?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, I didn't change the package.json file.
as I understand if we want to add that file we have to add them to package.json in section files, yes?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. So if it works correctly without having this index.d.ts file then why do we need it at all? Is it only needed for when developing it locally inside the library's repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe it would be easier to have that file in the project because it isn't included in the building process and doesn't change anything. But it's only my suggestion :D

Copy link
Member Author

@bohdanprog bohdanprog Jul 19, 2024

Choose a reason for hiding this comment

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

after a discussion about that with @jakex7 we decided to remove the index.d.ts file.

src/lib/resolveAssetUri.ts Show resolved Hide resolved
@bohdanprog bohdanprog requested a review from WoLewicki July 9, 2024 09:07
Copy link
Member

@WoLewicki WoLewicki left a comment

Choose a reason for hiding this comment

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

LGTM!

@bohdanprog bohdanprog merged commit 2da02cf into main Jul 19, 2024
1 check passed
@bohdanprog bohdanprog deleted the feat/resolve-web-asset branch July 19, 2024 15:12
jakex7 added a commit that referenced this pull request Jul 25, 2024
# Summary

* Export `Filter` and `FeColorMatrix` components on `web`
* Change filter IDs in example to be unique*
* Generate filter ID when using `FilterImage`*
* Hide `FilterImage` example on web, since it's crashing the whole site
(see #2334)

\* ID on web has to be unique, otherwise it'll use the first element
with that ID, even if they are in the separate SVG elements

## Test Plan

run `web-example` app

## Compatibility

| OS      | Implemented |
| ------- | :---------: |
| Web     |    ✅     |
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.

2 participants