Skip to content
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

externalizes core grunt methods to modules #1344

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jonschlinkert
Copy link

this is a wip!

@cowboy / @tkellen this is a first pass and is not ready to be merged. I wanted to start this pr to get discussion going.

Initially the goal was to only do file, but it turned out to not make much sense, since everything is so intertwined. e.g. instead of layering on more workarounds, I decided to try to decouple everything in one rough pass, expecting that through discussion we can improve some implementation specifics before this is ready to be merged.

I tried to minimize the changes necessary for decoupling the modules, but there are a couple of places where it couldn't be helped. For example:

  • in template, to decouple grunt I created a buildContext method that allows a custom function to be passed. This makes it pretty easy for users to modify how the data object is passed to _.template, but more importantly it allows this: https://github.com/jonschlinkert/grunt/blob/event-logger/lib/grunt/template.js#L16-L24. Which gives makes it work exactly as it does now.
  • regarding the event logger: it was setup to be generic, so that each grunt method uses a facade to proxy the module's log events to grunt.log methods. This ensures that each legacy lib uses its own instance of the event logger (so no double events are emitted), while still allowing grunt.log to have control over what's output to the console.

(TBC)

@@ -1,3 +1,4 @@
*.DS_Store
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to keep platform agnostic use a global .gitignore, see https://help.github.com/articles/ignoring-files/#create-a-global-gitignore

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did I create a new gitignore?! lol that was an accident. thanks for the link though! I'll remove it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:) thanks for working on this stuff by the way, looks great!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh nvm I see what you mean, yeah no problem I'll revert that commit.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@cowboy
Copy link
Member

cowboy commented Jun 5, 2015

For files like https://github.com/jonschlinkert/grunt/blob/event-logger/lib/grunt/cli.js that are entirely replaced by an external lib, I'd rather this code go into the main grunt.js and for the internal file to be removed.

@jonschlinkert
Copy link
Author

I'd rather this code go into the main grunt.js and for the internal file to be removed.

I wanted to change the least amount of code in lib/grunt.js possible until I got feedback. totally agree though

@jonschlinkert
Copy link
Author

If the order is changing with a new version of glob, we should consider using an older version, to avoid breaking backwards compatibility.

seems like this newer version of glob fixes #1338, but I'll defer to you on that.

@cowboy
Copy link
Member

cowboy commented Jun 5, 2015

@jonschlinkert, eventually #1338 can either be fixed or not fixed in its own PR, this PR shouldn't try to address any issues external to fixing #1329.

@tkellen
Copy link
Member

tkellen commented Jun 5, 2015

I think given the amount of change we're introducing here we should lock to the same deps.

@cowboy
Copy link
Member

cowboy commented Jun 5, 2015

I think given the amount of change we're introducing here we should lock to the same deps.

👍

@cowboy
Copy link
Member

cowboy commented Jun 5, 2015

@jonschlinkert I'm noticing that there's a lot going on in this PR.

I can see how breaking multiple modules out at the same time helps to inform what needs to be done since the modules are so tightly coupled, but I'm thinking that it might be easier to review and test this if the modules are replaced one at a time, instead of all at once.

What do you think?

@jonschlinkert
Copy link
Author

I think given the amount of change we're introducing here we should lock to the same deps.

I have no reason to want different dep versions, this is easy enough to fix. I was working off of this statement: "we just need to make sure that if we use a newer version of lodash, that our unit tests are comprehensive in case something changed with any of the methods we use." which doesn't necessarily imply that I "should" update the deps - or any deps besides lodash, but for some reason I internalized it that way..

@cowboy
Copy link
Member

cowboy commented Jun 5, 2015

@jonschlinkert I think for the sake of our collective sanity, at least for now, let's leave all the deps at their grunt 0.4.5 versions. It'll be once less thing to have to worry about.

@cowboy
Copy link
Member

cowboy commented Jun 5, 2015

I'm taking a shot at this in #1345, I have some ideas.

Base automatically changed from master to main March 22, 2021 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants