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

ehn(api): add unregisterLanguage method #3009

Merged
merged 4 commits into from Feb 15, 2021

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Feb 15, 2021

Changes

Currently, there is no way of un-do a registerLanguage operation except re-loading the whole library. Adds a unregisterLanguage method to the public API.

Checklist

  • No markup tests, this shouldn't change anything to the markup
  • Updated the changelog at CHANGES.md
  • Added myself to AUTHORS.txt, under Contributors

@joshgoebel
Copy link
Member

joshgoebel commented Feb 15, 2021

Curious: What is your use case for needing to unregister a language? It seem obvious yet we've gotten by without it since inception so I'm curious.

@joshgoebel
Copy link
Member

joshgoebel commented Feb 15, 2021

This would also need to be documented in api.rst. Otherwise looks about as I'd expect. :)

@joshgoebel joshgoebel added this to the 10.7 milestone Feb 15, 2021
@joshgoebel joshgoebel self-requested a review February 15, 2021 15:26
Copy link
Member

@joshgoebel joshgoebel left a comment

Choose a reason for hiding this comment

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

As this would be a core API method we'd also probably want at least one test to show that it indeed does what it's expected to do.

src/highlight.js Show resolved Hide resolved
@aduh95
Copy link
Contributor Author

aduh95 commented Feb 15, 2021

Curious: What is your use case for needing to unregister a language?

Useful for testing, in the test suite we have to remove the module from the cache to remove the test languages added by the other tests:

delete require.cache[require.resolve('../../build')];
delete require.cache[require.resolve('../../build/lib/core')];

This could be avoided if the other tests were cleaning up their test languages. This is needed to move the test suite to ESM syntax (joshgoebel#1).

@joshgoebel
Copy link
Member

This is needed to move the test suite to ESM syntax

Ah, that makes sense since we can't just delete the require cache anymore. :)

@joshgoebel joshgoebel merged commit 2f5590e into highlightjs:master Feb 15, 2021
@joshgoebel
Copy link
Member

@aduh95 Thanks!

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

2 participants