factor out shareable methods to a common class #140
Conversation
|
FYI, here is what adding the "direct" interface looks like: |
|
I'll try it out this weekend. Thanks for your effort! |
|
Hi @SpaceK33z. Please let me know if you have any concerns with this PR which I can address. Thanks. |
|
@grawk, sorry, I'm a bit busier than expected. I still have it in my list to review. |
| var MemoryFileSystem = require("memory-fs"); | ||
|
|
||
| module.exports = function shared(context) { | ||
| var shared = { |
There was a problem hiding this comment.
I've never been a fan of vars sharing the same name as the fn that contains them. Makes for some funky reading. would have chosen result here instead, but that's just a random's $0.02. 😄
There was a problem hiding this comment.
Would you like me to underbar the variable as _shared or un-name the function? Or some other resolution?
There was a problem hiding this comment.
I would say rename the function to Shared (note the capital) to be consistent with the other file, where you require it and call it Shared.
And maybe name the var something like share or config.
| if(options.lazy && (!options.filename || options.filename.test(filename))) | ||
| rebuild(); | ||
| if(context.options.lazy && (!context.options.filename || context.options.filename.test(filename))) | ||
| shared.rebuild(); | ||
|
|
||
| if(HASH_REGEXP.test(filename)) { |
There was a problem hiding this comment.
Could this be moved to the shared part completely? I feel like this should not be exposed.
| if(filename === false) return goNext(); | ||
|
|
||
| // in lazy mode, rebuild on bundle request | ||
| if(options.lazy && (!options.filename || options.filename.test(filename))) | ||
| rebuild(); | ||
| if(context.options.lazy && (!context.options.filename || context.options.filename.test(filename))) |
There was a problem hiding this comment.
Maybe also make a method for this in the shared file?
| options.log("webpack: bundle is now INVALID."); | ||
| } | ||
| }, | ||
| handleRangeHeaders: function handleRangeHeaders(content, req, res) { |
There was a problem hiding this comment.
This method assumes that res.setHeader, req.headers.range and res.statusCode work, but we don't know what server will be used, so this is a bit of a leaky abstraction. Don't know the correct way to do this though.
There was a problem hiding this comment.
I added a comment in the method, suggesting a way forward. E.g. conditional(s) based on an object check for various APIs. A new context flag could also be added to indicate what API is being used. But I believe that can be addressed in a future PR for support of additional server(s).
|
|
|
@grawk, could you sign the CLA? |
|
@SpaceK33z Hm. Somehow it seems these commits got associated with my corporate github ID. Therefore when I authorized the CLA app with my public github account, it's not able to link that with this changeset. Not sure how that happened :( Will look into it. |
|
I've associated my other email address (found in .gitconfig), [email protected], with my "grawk" account. It's not seemingly resolving the issue as far as the CLA app is concerned. Is there some other way I can verify to you I've signed the CLA or what resolution would you recommend? |
|
I think squashing or making a new PR with the correct account is best. Sorry for the trouble. |
|
OK. I was afraid you'd say that 😆 I'll probably have to table this today and complete it tomorrow. |
refactor util as shared and make pass lint uppercase module name change exported function name to Shared, inner object name to share addressing PR comments
|
squashed into a single commit, although the CLA badge is not reflecting a different status. edit. looks all good now :) |
|
Thanks, awesome work. I'll probably tweak some things here and there, but it looks good :). |
|
Thank you :) Happy to help. |
|
FYI, Published this in |
Leaving this here to get your impression of this approach. Using it, it would be fairly simple to add an additional interface to the module. One which simply takes a file name and returns the bundle content.