Useless insertBefore call
#1677
Conversation
components/prism-core.js
Outdated
| @@ -107,6 +107,10 @@ var _ = _self.Prism = { | |||
| var grammar = root[inside]; | |||
| var ret = {}; | |||
|
|
|||
| if (!grammar.hasOwnProperty(before)) { | |||
| throw new Error('Cannot insert before "' + before + '" in "' + inside + '"'); | |||
| } | |||
There was a problem hiding this comment.
I don't think I'd include this check, tbh, since this is something that should be controlled / fixed at "develop-time", rather than runtime.
There was a problem hiding this comment.
My only idea on how to check this at "develop-time" would be what I did here.
There was a problem hiding this comment.
Sorry, what I meant was that we should be verifying as we develop that we're not including unnecessary calls to insertBefore. Otherwise, we're shopping extra bytes that don't actually do anything at runtime.
There was a problem hiding this comment.
we should be verifying as we develop
Agreed, and the error is supposed to make all tests related to that language fail so that the dev notices that he/she broke something.
Example:
languages.foo = extend('bar', {});
insertBefore('foo', 'baz', { ... }); // 'baz' is defined in 'bar'If somebody then removes bar, that will break foo without him/her ever knowing.
Sure, tests are supposed to prevent that from happening but I think it's better to check at the source of the error.
Also, another reason why I think that check is necessary: How do you expect insertBefore to behave when before is not present in root[inside]?
My first guess wouldn't have been: It just creates a deep copy of root[inside] and updates all references it can find.
I actually expect this to throw an error in that case.
There was a problem hiding this comment.
The basic problem is that we're adding code to throw in a case that, if we code up the languages right, will never happen in production, so we're shipping code to end users that only benefits developers.
I understand the perspective of what's "right" for the function to do, but I think that only applies insofar as a function like this is primarily internal, rather than an API we are exposing to users.
There was a problem hiding this comment.
Then an insertBefore.test (like here) might be the best solution.
It behaves "right" during development but doesn't include the checks for the users.
There was a problem hiding this comment.
That would be better. Or a custom ESLint plugin that checks this (it should be verifiable statically).
There was a problem hiding this comment.
If you're planning on going the ESLint route, let's just merge this without the check and implement that separately.
There was a problem hiding this comment.
I'll just remove the check for now so we can merge this.
This PR removes the useless
insertBeforecall in Smarty.It didn't do anything because Smarty did not have a
tagtoken which causesinsertBeforeto just copy the whole language definition without doing anything.Why did Smarty have this in the first place?
I'm guessing that it's a leftover from before markup templating.
I also added a little check to catch cases like this in the future (throw, technically).