Conversation
|
That is a big PR. A lot of it is just change in codying style, which needlessly bloats this PR. Could you maybe separate those into its own PR? |
|
So split the code modernization into its own PR? |
f2e9444 to
c032c01
Compare
|
@StorytellerCZ Done! |
|
I'll open up the modernization PR once this one gets merged. 👍 |
StorytellerCZ
left a comment
There was a problem hiding this comment.
Looks good to me. I'm thinking that if we can combine this together with removal of jQuery and general modernization that you proposed, then we can do major version bump.
The removal of jQuery would definitely require a major version bump because as @jankapunkt pointed out people's Blaze code is making heavy use of internal jQuery method.
|
filipenevola
left a comment
There was a problem hiding this comment.
Hi @harryadel , thank you for the work here.
For the review to be effective we need to have PRs that are doing only one thing.
Could you keep only the underscore changes in this PR so we can move forward with it?
|
@filipenevola There's nothing but underscore changes. I've removed the modernization commits as Jan recommended. |
|
@harryadel I think the issue there is that your IDE or something automatic also "fixed" lot of the code around. |
|
As far as I saw, that was mostly lint fixes which is why I thought them to be ok |
|
I see so the problem lies with the extra linting. Ok, will remove it and notify you guys. |
|
@filipenevola I guess it's ready now |
packages/blaze/builtins.js
Outdated
| view.onViewCreated(function () { | ||
| _.each(bindings, function (binding, name) { | ||
| view._scopeBindings[name] = new ReactiveVar; | ||
| Object.keys(bindings).forEach(function (name) { |
There was a problem hiding this comment.
Object.entries is better in this case
| 'collection' | ||
| ); | ||
|
|
||
| eachView.argVar.set(arg); |
There was a problem hiding this comment.
is this change replacing underscore?
packages/blaze/util.js
Outdated
| @@ -0,0 +1,25 @@ | |||
| has = function (obj, key) { | |||
There was a problem hiding this comment.
we shouldn't expose anything globally, use exports / imports instead.
maybe it would be better to use specific modules of lodash instead of reimplementing these functions here.
|
@filipenevola friendly ping, please let me know what you think. |
filipenevola
left a comment
There was a problem hiding this comment.
There are still places where you should use a different Object utility and also some places where you should use lodash.
|
So the problem that was causing the site to fail was the non existence of files within the themes directory which is fixed by But there's a tiny problem with installing modules for the site though which I'll fix in a different PR. |
Please bear with me as this my first time contributing to the Blaze project.
The goal of this pull request is to:
Modernize the code baseBoth
test:ciandtest:watchyield no errors.That's for the code part but the site Hexo server isn't working. I noticed it reads jsdoc comments from several files like
builtins.js,preamble.jsand couple more so I reverted the linting on those files but then a weird error showed up in the browserI tried reverting the linting on the whole project and even updating the dependencies to the latest in attempt at least to get a more readable error but in vain.
I attempted to remove the underscore dependency from site directory but then reverted my changes so you guys can have easier time debugging things.