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

Fix resolve block to prefer custom extensions over defaults #177

Merged
merged 1 commit into from
Jul 30, 2017

Conversation

zcei
Copy link
Collaborator

@zcei zcei commented Jul 19, 2017

Allow custom extensions configurations, like

resolve({
  extensions: ['.custom.js']
})

so that

import foo from './foo'

first checks ./foo.custom.js before falling back to ./foo.js

Closes #176

@@ -1,5 +1,6 @@
const _ = require('lodash')
const webpackMerge = require('webpack-merge')
const webpackBlocksMerge = webpackMerge.smartStrategy({ 'resolve.extensions': 'prepend' })
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now I left it inlined, but in case there will be more custom behavior I'd recommend moving this object to something like merge-strategies.js

Copy link
Owner

Choose a reason for hiding this comment

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

I would say we should rather put this into resolve().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When putting it to resolve, we start introducing a lot of small different merge behaviors.
webpack-blocks allows you to define your own strategies and I think it makes sense to make use of it, as it alters only the object paths defined.

But yeah, we can still refactor this in case there are a lot of different merging strategies we want to have.
You could also argue as well that this isn't core behavior and some people might want to write something that actually replaces / appends / do other crazy stuff.

Going to change ✅

Copy link
Owner

@andywer andywer left a comment

Choose a reason for hiding this comment

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

We should put that logic into the resolve(). Putting the fix for resolve() so far away from it, under the hood, feels like a foot gun 😉

@@ -1,5 +1,6 @@
const _ = require('lodash')
const webpackMerge = require('webpack-merge')
const webpackBlocksMerge = webpackMerge.smartStrategy({ 'resolve.extensions': 'prepend' })
Copy link
Owner

Choose a reason for hiding this comment

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

I would say we should rather put this into resolve().

@andywer
Copy link
Owner

andywer commented Jul 19, 2017

Thanks for your PR, @zcei!

@zcei zcei force-pushed the fix/prepend-to-resolve-extensions branch from 2415996 to 9a38a52 Compare July 19, 2017 11:09
@zcei
Copy link
Collaborator Author

zcei commented Jul 19, 2017

@andywer I exposed a new util fn, that allows merging with given strategy, to not duplicate the webpack-merge dependency from core into the webpack block.

And it makes it very clear in every single block how you alter the default behavior.

WDYT?

@andywer
Copy link
Owner

andywer commented Jul 19, 2017

I get your intention, but my personal aim is to keep the utils as slim as possible. As you might know there is already a long-running discussion if util functions should be passed to the blocks at all.

Duplicating the webpack-merge dependency to @webpack-blocks/webpack might not look to elegant, but still seems like the smaller evil, compared to extending the utils by a function that is only used in one spot.

After all: No one said you have to stick to the utils to do the merging. In fact the 1.0 API blackboxes the way a block applies its changes to the config, so if you need something special (like here), feel free to do whatever you need to do in the block :)

@andywer
Copy link
Owner

andywer commented Jul 20, 2017

Btw, sorry for the wall of text 😉

I would open a PR with my solution and let you review it as soon as I find a few minutes time, but I wanted to wait for your response/feedback or that of my fellow co-contributors first.

@zcei
Copy link
Collaborator Author

zcei commented Jul 20, 2017

Yes, I was actually looking into putting it into the webpack block, but wanted to write a test for it then there as well and didn't get enough time to do so.

Happy to review other approaches, but better not double the work 🙂

@andywer
Copy link
Owner

andywer commented Jul 21, 2017

Sure, let's not double the work. Update the PR when you got time. But if you are just lacking the unit tests, don't mind; I would add them as well :)

@zcei zcei force-pushed the fix/prepend-to-resolve-extensions branch from 9a38a52 to f023ecc Compare July 29, 2017 20:51
@zcei
Copy link
Collaborator Author

zcei commented Jul 29, 2017

Sorry, was a bit short on time, but pushed what I got now, including a bare-minimal test to actually check the prepend behavior.

@andywer andywer merged commit 3ed3bfd into andywer:release/1.0 Jul 30, 2017
andywer added a commit that referenced this pull request Jul 30, 2017
@zcei zcei deleted the fix/prepend-to-resolve-extensions branch July 31, 2017 10:13
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.

2 participants