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

Allow already injected to be reset #380

Merged
merged 8 commits into from
Aug 28, 2019
Merged

Allow already injected to be reset #380

merged 8 commits into from
Aug 28, 2019

Conversation

milesj
Copy link
Contributor

@milesj milesj commented Jul 2, 2019

to: @lencioni

We have some logic in our app that deletes injected CSS rules (mostly global styles). This works great.

However, once the styles are regenerated and attempt to inject again, they will not as Aphrodite blocks the injection because the previous key is set to alreadyInjected. https://github.com/Khan/aphrodite/blob/master/src/inject.js#L159

This PR adds a minor function called resetInjected to clear this cache. I also went ahead and exported reset since it may also be useful.

Not sure how to test this since all the logic is abstracted away.

@milesj
Copy link
Contributor Author

milesj commented Jul 8, 2019

@lencioni You have any feedback?

Copy link
Collaborator

@lencioni lencioni left a comment

Choose a reason for hiding this comment

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

Can someone from Khan please take a look as well?

src/inject.js Outdated Show resolved Hide resolved
dist/aphrodite.js Outdated Show resolved Hide resolved
src/exports.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@lencioni lencioni left a comment

Choose a reason for hiding this comment

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

Can you please add a test?

README.md Outdated Show resolved Hide resolved
src/inject.js Show resolved Hide resolved
@milesj
Copy link
Contributor Author

milesj commented Jul 8, 2019

@lencioni Not sure how to test this since alreadyInjected isn't exported, and exporting a let var doesn't seem like a good idea.

@lencioni
Copy link
Collaborator

lencioni commented Jul 9, 2019

@milesj how about an integration test that uses aphrodite to inject styles into the DOM, and then removes the styles, and then calls your new function, and then uses aphrodite to inject that style into the DOM again and verify that all worked?

@milesj
Copy link
Contributor Author

milesj commented Jul 9, 2019

@lencioni Added.

@lencioni
Copy link
Collaborator

Thanks for the test! Do you know why CI is failing on Node 6?

@milesj
Copy link
Contributor Author

milesj commented Jul 10, 2019

@lencioni I do not but will look into it.

I'd also suggest just dropping v6 since it's LTS is over.

@milesj
Copy link
Contributor Author

milesj commented Jul 16, 2019

@lencioni @kevinbarabash Any update here?

@lencioni
Copy link
Collaborator

Thanks for the ping! I'd really like for someone from Khan to review this. @kevinbarabash can you help with that?

@milesj
Copy link
Contributor Author

milesj commented Aug 19, 2019

Bump? @kevinbarabash

@lencioni lencioni merged commit 06a881b into Khan:master Aug 28, 2019
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.

None yet

3 participants