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

Hot Reloading: Include Extension in Theme File Path #44

Merged

Conversation

tylerkrupicka
Copy link
Contributor

@tylerkrupicka tylerkrupicka commented Jan 5, 2021

What Changed

I changed the getThemeFilename function to include a file extension which was needed for hot reloading.

Why

There was recently a change to add the theme file as a dependency for hot reloading. I was having trouble getting this working in a design system and started debugging what was going on. What I found was the hot reloading logic used getThemeFilename to add the theme file as a dependency. However, the getThemeFilename function was returning the filename without an extension.

my-ds/components/Component/src/theme

I think this works in other parts of the project because require figures out the extension.

I edited node_modules and changed getThemeFilename to test for our default ts and js extensions, and my hot reloading started working. I'll test the change here with a canary.

Todo:

Published PR with canary version: 2.4.1-canary.44.566

@tylerkrupicka tylerkrupicka added the patch Increment the patch version when merged label Jan 5, 2021
@tylerkrupicka
Copy link
Contributor Author

This change is tested and working in my design system project.

@tylerkrupicka tylerkrupicka merged commit ef0ce02 into intuit:master Jan 5, 2021
@tylerkrupicka tylerkrupicka deleted the hot-reload-file-extension branch January 5, 2021 21:27
@hipstersmoothie
Copy link
Contributor

🚀 PR was released in v2.4.1 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants