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

Major feature: match() #163

Merged
merged 18 commits into from
Jun 10, 2017
Merged

Major feature: match() #163

merged 18 commits into from
Jun 10, 2017

Conversation

andywer
Copy link
Owner

@andywer andywer commented Jun 1, 2017

This is quite a big pull request (sorry!), but it comes with a bunch of neat improvements, I think:

  • Introduces match() to get rid of inconsistent exclude/include options in blocks
  • API of most blocks didn't change or just dropped exclude/include options
  • API of @webpack-blocks/assets (css(), file() & url() block) changed noticeably
  • Docs have been updated

Needed to touch all blocks for this (😱), but I used this opportunity to improve the block tests a bit and add a few new tests. Also cleaned up some nasty code bits.

Please have a look, do a quick check of the changed code and read the updated docs if you can find some time. I would like to publish this as 1.0.0-beta.

Shout if you disagree with something, I was out on a limb here 😉

@andywer andywer mentioned this pull request Jun 1, 2017
@andywer
Copy link
Owner Author

andywer commented Jun 5, 2017

I know I touched a lot of stuff, but I hope you will find some time to quick-review the PR nevertheless.

If you are in a hurry, just check out the changed markdown files to see what changes for the user instead of reviewing the under-the-hood changes 😉

@jvanbruegge @sapegin @aaronjensen @eXon @diegohaz @bkonkle

* `exclude`: Condition(s) which paths not to match. Might not be present.
* `include`: Condition(s) to override `exclude`. Might not be present.

If the block is not used within a `match()` then `context.match` will be undefined.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

])
})

test('Postcss works with css() & match()', t => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a sass & match test case

Copy link
Owner Author

Choose a reason for hiding this comment

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

Sure 👍

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done. But I added it to the global integration test (packages/webpack/__tests__/integration.test.js).

@andywer andywer force-pushed the feature/match-filetypes branch from 28b238e to 86bcdb7 Compare June 6, 2017 13:31
@andywer
Copy link
Owner Author

andywer commented Jun 7, 2017

Last chance to review this PR. Going to merge it today or tomorrow.

Will publish v1.0.0-beta thereafter 🎉

@sapegin
Copy link
Collaborator

sapegin commented Jun 7, 2017

I think we should get rid of context.fileType entirely — I don’t see any benefits of having it now. We could just use globs everywhere and replace it with something like context.testFor('*.css').

@andywer
Copy link
Owner Author

andywer commented Jun 7, 2017

Fair enough. I am not sure if we even need a context.testFor(). Could just use test: context.match.test || /\.css$/.

In this case we would just change our loaders and deprecate context.fileType, yielding a warning if you still use it.

@sapegin
Copy link
Collaborator

sapegin commented Jun 7, 2017

If it’s only for custom loaders then it’s good enough ;-)

@andywer
Copy link
Owner Author

andywer commented Jun 7, 2017

What do you mean by "if it’s only for custom loaders"?

@sapegin
Copy link
Collaborator

sapegin commented Jun 7, 2017

Blocks, not loaders ;-)

@bkonkle
Copy link

bkonkle commented Jun 9, 2017

I think we should get rid of context.fileType entirely

I agree - there's not really a need for it any more, and it unnecessarily complicates things in my opinion. I like the match() api much better. 👍

@andywer
Copy link
Owner Author

andywer commented Jun 9, 2017

I pushed a commit to replace all usages of context.fileType in webpack-blocks and mention in the docs and code that it is deprecated.

I'd say this can now be published as 1.0 beta. We can introduce a warning message in the next version and remove it completely in the version thereafter.

@bkonkle
Copy link

bkonkle commented Jun 9, 2017

Awesome! 🎉

@andywer andywer merged commit f394021 into release/1.0 Jun 10, 2017
@andywer andywer deleted the feature/match-filetypes branch June 10, 2017 14:40
@andywer
Copy link
Owner Author

andywer commented Jun 12, 2017

Published as v1.0.0-beta

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