-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fixing #504. Jspm compatibility and static declaration of dependencies #614
Conversation
@@ -73,8 +118,11 @@ function preparePluginsArray(plugins) { | |||
plugin = setupCustomPlugin(key, item[key]); | |||
|
|||
} else { | |||
if (!pluginMap[key]) { | |||
throw new Error("Unknown plugin '" + key + "'"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to maintain a similar behavior to what happens when you try to require
a plugin that doesn't exist. Now that the plugins are not being require
d in this function, this Error maintains backwards compatibility of what should happen for non-existent plugins.
@@ -73,8 +118,11 @@ function preparePluginsArray(plugins) { | |||
plugin = setupCustomPlugin(key, item[key]); | |||
|
|||
} else { | |||
if (!pluginMap[key]) { | |||
throw new Error('Unknown plugin "' + key + '"'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to maintain a similar behavior to what happens when you try to require a plugin that doesn't exist. Now that the plugins are not being required in this function, this Error maintains backwards compatibility of what should happen for non-existent plugins.
Looks like the build is failing only in node 6. The tests passed but after the tests coverall failed and I'm not sure exactly why. Any ideas on why I'm getting |
That happens sometime. Dont't worry, it should be ok. |
I don't like the implementation since it loads all plugins including non-used one. It'll certainly slow down svgo execution for all users which I cannot accept. Also I cannot verify that it solves the issue. Is there a less kludgy solution? |
This fixes #504. There are definitely pros and cons to doing this, and am interested to hear your thoughts on what is best for the svgo project. My opinion, of course, is that this the pros outweigh the cons, but I can see the other side too.
Pros:
import
,System.import()
, orimport()
, you can no longer synchronously load dynamic plugin code like you can withrequire
. Instead, you can either import all of the plugins upfront (similar to what I've done in this pull request) or you can change the preparePluginsArray function to be an asynchronous function that returns a System.import() promise.Cons:
pluginMap
is sort of an annoying manifesty thing that introduces one more step to the process of writing a new svgo plugin.