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

Support for ESLint #254

Merged
merged 8 commits into from
Mar 2, 2018
Merged

Support for ESLint #254

merged 8 commits into from
Mar 2, 2018

Conversation

dmitmel
Copy link
Contributor

@dmitmel dmitmel commented Feb 20, 2018

Almost like the tslint block, but for JavaScript.

@@ -0,0 +1,35 @@
# webpack-blocks - TSLint
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy 🍝 - needs to be ESLint as well 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, thanks :) These packages are very similar, so I've decided to copy files from the tslint block.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, that's how I would have done as well 😉

@andywer
Copy link
Owner

andywer commented Feb 22, 2018

Proposal: Let's extract the tslint block out of the core webpack-blocks repo into its own and let's put the eslint block into a separate repo as well.

We can still link to them in the README here, but trimming down the number of packages here might be a good idea. Thoughts?

@jvanbruegge

@dmitmel
Copy link
Contributor Author

dmitmel commented Feb 22, 2018

Well, I think these packages must be included in the webpack-blocks package because they are used in lots of projects (see their stats). But, your CI can't run all tests because there are too many packages. An ideal solution would be creating an organization and putting all packages into separate repositories.

@jvanbruegge
Copy link
Contributor

@andywer
Would be ok with that, just dont forget to make a major version bump on the webpack-blocks package. I dont want all the setups suddenly breaking

@zcei
Copy link
Collaborator

zcei commented Feb 22, 2018

I would really like to see most blocks in their own repo - that's why I initially blocked the webpack-blocks org org.
Though I think especially ESLint (& TSLint ?) should be treated first-class as they're nearly always used.

@andywer
Copy link
Owner

andywer commented Feb 23, 2018

Then maybe let's postpone this decision (extra repo or not) for now 😉

@jvanbruegge That shouldn't be a breaking change. Should have no influence on user's where the sources reside.

@dmitmel Yeah, but the block's are more utility than core functionality and the download count of the tslint block is quite low.

The CI issue is quite annoying, though... We could try lerna run --parallel or lerna run --stream to circumvent that issue or try a different monorepo tool.

@dmitmel
Copy link
Contributor Author

dmitmel commented Feb 25, 2018

@andywer Would you like to add the eslint block or remove the tslint block?

@andywer
Copy link
Owner

andywer commented Feb 25, 2018

@dmitmel Let's add the eslint block for now and decide on the overall strategy afterwards. That discussion shouldn't be a reason to block new features :)

andywer
andywer previously approved these changes Feb 26, 2018
@@ -0,0 +1,5 @@
# @webpack-blocks/eslint - Changelog

## Next release
Copy link
Owner

Choose a reason for hiding this comment

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

In this case we can enter the version number already, since we will publish right away.

andywer
andywer previously approved these changes Feb 26, 2018
@vlad-zhukov
Copy link
Collaborator

vlad-zhukov commented Feb 26, 2018

I'd move unpopular and framework-specific blocks out of the main repo (for example elm). It will make installs and tests faster (good for both development and CI), but most importantly you can grant write access to repos to people who care about the block, and don't stop their development interests.

@vlad-zhukov
Copy link
Collaborator

@dmitmel Thanks for the PR! Would be great to have a few test cases similar to ones other blocks have.

@dmitmel
Copy link
Contributor Author

dmitmel commented Mar 2, 2018

@vlad-zhukov I've written some integration tests. Thanks for pointing out!

Copy link
Collaborator

@vlad-zhukov vlad-zhukov left a comment

Choose a reason for hiding this comment

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

LGTM.

Nit: I'd also add an example and a test with a babel block as it looks like it's a popular setup.

match('.js', [
	eslint(),
	babel(),
])

But we can do it later.

@andywer
Copy link
Owner

andywer commented Mar 2, 2018

Awesome, thanks for your efforts! 😊

@andywer andywer merged commit 1e0cdd6 into andywer:master Mar 2, 2018
@andywer
Copy link
Owner

andywer commented Mar 2, 2018

Published as @webpack-blocks/eslint@1.0.0-rc 🚀

@dmitmel dmitmel deleted the feature/eslint-block branch March 2, 2018 09:53
@dmitmel dmitmel restored the feature/eslint-block branch March 2, 2018 09:55
@dmitmel
Copy link
Contributor Author

dmitmel commented Mar 2, 2018

I've found something wrong in the eslint block: it has @webpack-blocks/core in its peer dependencies.

@andywer
Copy link
Owner

andywer commented Mar 2, 2018

Might be a copy-pasta mistake. I don't remember with certainty, but I think at some point we added that peer dependency to make explicit that the block only works with webpack-blocks v1 (not v0.x).

It's not critical and the peer dependency is basically true 😉

@dmitmel
Copy link
Contributor Author

dmitmel commented Mar 2, 2018

@andywer Nice hack workaround!

@dmitmel dmitmel deleted the feature/eslint-block branch March 2, 2018 21:09
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.

5 participants