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

fix(plugins): fix file loader behavior [LIBS-399] #779

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

KaiVandivier
Copy link
Contributor

@KaiVandivier KaiVandivier commented Jan 16, 2023

Fixes https://dhis2.atlassian.net/browse/LIBS-399

It seems like the file-loader is mangling the css file from maplibre-gl (which doesn’t need compiling, strangely) and outputting all kinds of nasty junk that doesn’t work, like:

  • static/media/javascript,__webpack_public_path__=….bin
  • static/media/svg+xml;base64,ASD:LKFDNAJKSD….bin
  • other nasty .bin files

In the maps app, this caused an "Filename too long" error that prevented building the app, and svg icons were broken when run in dev mode.

Changing to the modern asset/resource "file loader" seems to fix it with no noticeable side effects.

This configuration was originally what I used when first making plugins, but I seem to remember there was a reason not to use it -- I think it had to do with CSS modules, but this config seems to be running and building fine in the PWA test app, the LL app, and the Maps app, so I don't see any reason not to use it now 🤷‍♂️

@KaiVandivier KaiVandivier requested a review from a team January 16, 2023 16:07
@KaiVandivier
Copy link
Contributor Author

KaiVandivier commented Feb 14, 2023

@jenniferarnesen has confirmed that this fix works in the maps app 👍 (see the slack thread mentioned in the Jira issue above)

Copy link
Contributor

@HendrikThePendric HendrikThePendric left a comment

Choose a reason for hiding this comment

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

I left a comment because one part was unclear to me. Perhaps I am simply failing to see what's going on. So I've just left a comment, instead of approving or requesting changes....

exclude: [
/^$/,
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this newly added regex only matches an empty string. I wonder if this is actually useful, or perhaps a remnant of some early stage experimentation.....
Screenshot 2023-02-16 at 15 05 24

Could you explain why this is needed here?

Copy link
Contributor

@HendrikThePendric HendrikThePendric left a comment

Choose a reason for hiding this comment

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

I'm approving after a brief discussion on Slack. @KaiVandivier and I both do not fully understand why it might be useful to exclude empty strings in the file loader. But since CRA also include this one, we might as well do the same.

@KaiVandivier KaiVandivier merged commit dcdd918 into master Feb 16, 2023
@KaiVandivier KaiVandivier deleted the libs-399-fix-file-loader branch February 16, 2023 14:46
dhis2-bot added a commit that referenced this pull request Feb 16, 2023
## [10.2.3](v10.2.2...v10.2.3) (2023-02-16)

### Bug Fixes

* **plugins:** fix file loader behavior ([#779](#779)) ([dcdd918](dcdd918))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 10.2.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants