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

remove() end early #34

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

singingwolfboy
Copy link

I was having a problem while using Squire.js with my test suite, where the context was undefined when I called injector.remove(). This modification fixed it. I also removed trailing whitespace in a separate commit.

@iammerrick
Copy link
Owner

Could you please example the problem better?

@fearphage
Copy link

The problem is Squire throws if remove() is called when there is nothing to remove. new Squire().remove() should not throw. It currently does.

@fearphage
Copy link

I also noticed remove is not tested at all. Might explain the issue. @singingwolfboy mind adding tests?

@fearphage
Copy link

The same is true for clean(). new Squire().clean() throws because the context is undefined.

@iammerrick
Copy link
Owner

@singingwolfboy Could you add a test around this? I'm struggling to know when you wouldn't have a context?

@RLovelett
Copy link

I can't exactly explain why context is ending up undefined (yet). I can however say this does fix the issue.

RLovelett added a commit to RLovelett/Squire.js that referenced this pull request Apr 17, 2014
@RLovelett
Copy link

@iammerrick The issue was that inside of Squire#remove there is a call to completely destroy the current Squire.js context that RequireJS was managing. This context was the context created during the constructor of the Squire instance.

I have proposed a solution, in pull request #49. The solution implemented in this patch is to rebuild a new RequireJS context, after deleting the initial context. This way the RequireJS context is fresh, as the remove call would indicate it should be, while also still being defined for subsequent calls.

Additionally, there should no longer be an edge case that leads to the context being undefined in the first place, as was the initial symptom of the issue.

@shustariov-andrey
Copy link

Having the same issue. And this pull request fixes it in my case.

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.

5 participants