Skip to content

remove() end early - #34

Open
singingwolfboy wants to merge 2 commits into
iammerrick:masterfrom
singingwolfboy:remove-end-early
Open

remove() end early#34
singingwolfboy wants to merge 2 commits into
iammerrick:masterfrom
singingwolfboy:remove-end-early

Conversation

@singingwolfboy

Copy link
Copy Markdown

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
Copy Markdown
Owner

Could you please example the problem better?

@fearphage

Copy link
Copy Markdown

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
Copy Markdown

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

@fearphage

Copy link
Copy Markdown

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

@iammerrick

Copy link
Copy Markdown
Owner

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

@RLovelett

Copy link
Copy Markdown

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
Copy Markdown

@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
Copy Markdown

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