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 duplicate files not imported, and other fixes #304

Merged
merged 6 commits into from
May 17, 2021
Merged

Fix duplicate files not imported, and other fixes #304

merged 6 commits into from
May 17, 2021

Conversation

sebastian-ruiz
Copy link
Contributor

This should fix at least issues: #289, #246, #239 and #253.

The main inspiration for this fix comes from: sass/node-sass#2556

which says to add file parameter even when passing comment, like so:

done({ contents: fs.readFileSync(parsed.path, 'utf8'), file: parsed.path});

@copleykj copleykj requested a review from sebakerckhof May 5, 2021 20:28
@sebakerckhof
Copy link
Collaborator

Hi @sebastian-ruiz . Thanks for the PR.
However, I do remember trying setting both file and data and that there used to be issues with this: sass/node-sass#1195 . But that's a long time ago and I'll check again if this is now a valid approach. It would indeed help us determine which file is being processed, since now sometimes it comes down to guessing.

@sebastian-ruiz
Copy link
Contributor Author

sebastian-ruiz commented May 6, 2021

Yes I saw sass/node-sass#1195. I assume things changed between then and sass/node-sass#2556 which allows for this fix.

With this PR I'm able to use stuff like bootstrap, font-awesome, and react libraries from NPM together without issues. This wasn't possible before because of conflicting variables.scss files and things like that.

@sebakerckhof
Copy link
Collaborator

sebakerckhof commented May 17, 2021

Looks like providing both contents and file now works indeed. Nice find. I think with that the code could be simplified quite a bit. But for don't really have time to look into this further at the moment. Tests pass locally, so I'll release this as is so those bugs are fixed at least.

@sebakerckhof sebakerckhof merged commit 076d365 into Meteor-Community-Packages:master May 17, 2021
@sebakerckhof
Copy link
Collaborator

Included in 4.14.1, thanks

This pull request was closed.
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