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

Fixing resolve issue with bootstrap-sass #342 #343

Closed
wants to merge 1 commit into from

Conversation

andrei1489
Copy link

@andrei1489 andrei1489 commented Apr 10, 2018

This change is Reviewable

@ttma1046
Copy link

I don't think a hardcode version of resolve can be treated as a fix.

@andrei1489
Copy link
Author

@ttma1046 I added this just to keep it working while the project is updated to use resolve@1.7.0. Having the latest version of something is not necessarily a good practice, as issues like the one we are facing can happen. It really sucked having my pipeline break because of an external dependency.

@justin808
Copy link
Member

@alexfedoseev Any opinion on this one? What if resolve 1.8.0 fixes this issue and we're pinned to 1.6.0?

@ttma1046
Copy link

@andrei1489

Since the resolve@1.7.0 changed the return result of the sync() function.
The proper fix of this issue is suppose to be changing resolveModule.js in bootstrap-loader to handle the new return result from resolve.sync().

@andrei1489
Copy link
Author

andrei1489 commented Apr 12, 2018

@ttma1046 I agree. This fix was intended to keep projects from breaking while the proper fix is being worked on. For my project I had to add resolve as a depencendy to make it work, just as you did.

EDIT: browserify/resolve#157 there are discussions to keep the return format for 1.x branch and add a 2.x branch with the new format.

@ljharb
Copy link

ljharb commented Apr 12, 2018

v1.7.1 will fix this issue soon.

@justin808
Copy link
Member

@ljharb @andrei1489 @ttma1046 should we close this PR?

@andrei1489
Copy link
Author

Hi @justin808 , since resolve 1.7.1 came out, I will close it.

@andrei1489 andrei1489 closed this Apr 22, 2018
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