feat: Allow loading nested objects onto existing libraries #45
Conversation
|
|
TiraO
force-pushed
the
nested-existing-libraries
branch
from
35fd055
to
8063afd
Compare
michael-ciniawsky
requested review from
bebraw,
michael-ciniawsky and
joshwiens
test/test.js
Outdated
| @@ -20,11 +20,11 @@ describe("loader", function() { | |||
| }, "").should.be.eql(HEADER + | |||
| // First import | |||
| "var abc = (abc || {});\n" + | |||
| "abc.def = {};\n" + | |||
| "abc.def = abc.def || {};\n" + | |||
There was a problem hiding this comment.
Maybe put this in a additional test instead of changing the existing one, if this makes no difference and covers the current purpose of the test, feel free to ignore me here :)
joshwiens
changed the title
test/test.js
Outdated
| @@ -9,7 +9,7 @@ describe("loader", function() { | |||
| query: "?abc.def.ghi=>1" | |||
| }, "").should.be.eql(HEADER + | |||
| "var abc = (abc || {});\n" + | |||
| "abc.def = {};\n" + | |||
| "abc.def = abc.def || {};\n" + | |||
There was a problem hiding this comment.
@michael-ciniawsky is correct. This should be two tests, the existing plus an additional test to cover the new nesting functionality
There was a problem hiding this comment.
It does seem like there should be a test that is not impacted by this change. That probably means adding a test for the case where there is no nesting, or exec the generated code and test its behavior:
describe("when the import is nested", function(){
it("creates the nested variable", function(){
....
});
describe ("when the import is a plugin to an existing library", function(){
it ("adds the functionality to the library", function (){
expect(some.nested.library.originalProperty).toBeDefined ()
...That would be a larger refactor to how the tests are structured.
There was a problem hiding this comment.
This is my fault for not paying attention when I looked at it originally but as a life rule in JavaScript, don't use eval. Even when it isn't a gigantic gaping security issue, there is always a way to do the same thing without it.
- now tests cover the generated code's behavior instead of testing the literal text of the code
There was a problem hiding this comment.
@TiraO Thx 👍 && very much 💯 for the addtional test updates a lot better now
|
@TiraO Can you give me a brief usage example for the docs please, I will overhaul the README today before next release |
e.g. loading a jquery plugin jquery.event.drag should not set event.drag to empty object.