-
Notifications
You must be signed in to change notification settings - Fork 8
Add require JS. #36
base: master
Are you sure you want to change the base?
Add require JS. #36
Changes from all commits
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 |
---|---|---|
@@ -1,4 +1,6 @@ | ||
<!doctype html> | ||
<html> | ||
<script src="lib/require.min.js"></script> | ||
<script src="lib/requireConfig.js"></script> | ||
<script src="background.js"></script> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,23 +1,31 @@ | ||
chrome.pageAction.onClicked.addListener(function(tab) { | ||
chrome.tabs.executeScript(null, { file: "src/startContentProcessor.js" }); | ||
}); | ||
requirejs.config(requirejsConfig); | ||
|
||
// When the extension is installed or upgraded ... | ||
chrome.runtime.onInstalled.addListener(function() { | ||
// Replace all rules ... | ||
chrome.declarativeContent.onPageChanged.removeRules(undefined, function() { | ||
// With a new rule ... | ||
chrome.declarativeContent.onPageChanged.addRules([ | ||
{ | ||
// That fires when a page's URL contains a 'g' ... | ||
conditions: [ | ||
new chrome.declarativeContent.PageStateMatcher({ | ||
pageUrl: { hostEquals: 'workflowy.com', schemes: ['https', 'http'] }, | ||
}) | ||
], | ||
// And shows the extension's page action. | ||
actions: [ new chrome.declarativeContent.ShowPageAction() ] | ||
} | ||
]); | ||
}); | ||
}); | ||
requirejs( | ||
{ baseUrl: chrome.extension.getURL("/") }, | ||
["src/startContentProcessor"], | ||
function (Processor) { | ||
chrome.pageAction.onClicked.addListener(function(tab) { | ||
Processor(); | ||
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 should probably be a verb. |
||
}); | ||
|
||
// When the extension is installed or upgraded ... | ||
chrome.runtime.onInstalled.addListener(function() { | ||
// Replace all rules ... | ||
chrome.declarativeContent.onPageChanged.removeRules(undefined, function() { | ||
// With a new rule ... | ||
chrome.declarativeContent.onPageChanged.addRules([ | ||
{ | ||
// That fires when a page's URL contains a 'g' ... | ||
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. Pretty sure this comment's wrong. |
||
conditions: [ | ||
new chrome.declarativeContent.PageStateMatcher({ | ||
pageUrl: { hostEquals: 'workflowy.com', schemes: ['https', 'http'] }, | ||
}) | ||
], | ||
// And shows the extension's page action. | ||
actions: [ new chrome.declarativeContent.ShowPageAction() ] | ||
} | ||
]); | ||
}); | ||
}); | ||
} | ||
); |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
var requirejsConfig = { | ||
baseUrl: '/src', | ||
paths: { | ||
lib: '/lib' | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
(function() { | ||
var global = this; | ||
require.load = function (context, moduleName, url) { | ||
var xhr; | ||
xhr = new XMLHttpRequest(); | ||
xhr.open("GET", chrome.extension.getURL(url) + '?r=' + new Date().getTime(), true); | ||
xhr.onreadystatechange = function (e) { | ||
if (xhr.readyState === 4 && xhr.status === 200) { | ||
eval.call(global, xhr.responseText + '\n//@ sourceURL=' + url); | ||
context.completeLoad(moduleName) | ||
} | ||
}; | ||
xhr.send(null); | ||
}; | ||
})(); | ||
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 is weird. Why is this? The It looks like you're changing how require.load works to use AJAX instead of it's built-in mechanisms. That will work fine but it's pretty weird and should be unnecessary; normal loading should work as-is, even in a Chrome extension. Happy to stick with it if you've got a good reason, but it'd be nice to document that reason here. 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. Er, I copied this from somewhere sorry... Sam C and I were googling around how to add require JS to a chrome extension, it seemed that something along these lines was necessary but I didn't quite figure out why :/ Found it - https://prezi.com/rodnyr5awftr/requirejs-in-chrome-extensions/ 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. Ah, interesting. I see: this is so that RequireJS works inside content scripts. Now I look closer, I can see that it only ever gets loaded inside the content scripts, and never in the normal app (where it won't work, because eval isn't allowed by default). That makes sense, as is fine, but we should document it. I'd stick a comment in here referencing that link and explaining that this is needed because the normal requirejs loading mechanism doesn't work in content scripts, and I'd rename this file to something like "patch-require-for-content-scripts" so it's clearer what's going on at a quick glance. Otherwise this is fine :-) |
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.
It's much more normal to include this call in the config file itself I think. Makes it clearer how that other file is used, and stops this file depending on extra globals existing.