-
Notifications
You must be signed in to change notification settings - Fork 39
feat(tag-appender): Use requirejs to load js files if avail #14
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
Changes from all commits
7415153
1f6fcd1
c893a02
c57f044
f6fb87e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ | |
"_": false, | ||
"angular": false, | ||
"require": false, | ||
"module": false | ||
"module": false, | ||
"window": false | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,7 @@ | |
"using": false, | ||
"pause": false, | ||
"resume": false, | ||
"sleep": false | ||
"sleep": false, | ||
"window": false | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
window.lazyLoadingWorking = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This effects global state, so once you set it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can give a try to requirejs.undef to see it that triggers the module loading again when requested. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
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.
Sorry, I didn't understand why this
headElement
check is necessary...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.
First of all because I don't know if there are current scenarios where you're changing the element where to insert the scripts.. It's in a value so everyone can decorate or override it and I didn't want to introduce a back compatibility issue, as RequireJS will not respect the value of headElement and will insert the scripts always in
document.head
.But also it is very convenient in order to avoid breaking the current tests (that where injecting a mock instead of headElement) as RequireJS will be present in all the tests.
I considered adding some kind of flag or configuration option to toggle requirejs on and off, but then I realized that the headElement is a nice toggle for testing purposes while also saving back-compatibility.
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.
I think it is better to use $window.requirejs and then you can mock $window to have or to not to have requirejs. I'll patch this up when merging later today. Thanks!
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.
That sounds good to me. The bad thing is that it's going to affect all the other tests, but I guess it's o.k. Let me know if you want me to make it instead.
Thanks to you!