Skip to content

Conversation

@illusionfield
Copy link
Contributor

@illusionfield illusionfield commented Apr 10, 2025

Closing this PR in favor of the consolidated version here:
➡️ #329 – Finalize Dart Sass migration & drop direct dependency

That PR targets release/5.0.0 and includes all changes from this one, along with updated docs and migration instructions.
Looking forward to your feedback there!


This PR introduces v5.1.0-beta.1 with a key internal change:
sass is no longer declared as a direct dependency.
Instead, users can now choose their own Sass engine (sass or sass-embedded) and version via npm.


What's Changed

  • 🔧 Removed built-in dependency on sass
    Gives full control to developers to pin versions, choose implementation, or upgrade independently.

  • 📚 Improved README.md

    • Clarified setup instructions for both sass and sass-embedded
    • Reorganized sections for better readability
    • Added guidance about imports from outside the Meteor project and file watcher limitations
  • 🆙 Version bump to v5.1.0-beta.1
    Reflects an internal change that may affect build behavior for some users.


Migration Notes

Users upgrading to v5.1.0-beta.1 must now manually install a Sass compiler:

meteor npm install --save-dev sass
# or
meteor npm install --save-dev sass-embedded

Why This Matters

This decoupling makes the package:

  • more flexible for real-world projects,
  • more future-proof (e.g. for using sass-embedded),
  • and easier to maintain with Meteor 3 and beyond.

illusionfield and others added 27 commits February 7, 2025 15:32
* master:
  Internal Meteor Packages: improve root file detection logic for imports and package files Meteor-Community-Packages#324
Addressed an issue where running `meteor test-packages` in Meteor 2 failed due to npm dependency installation errors related to the `meteor-node-stubs` update. This fix ensures compatibility and resolves the 'npmignore: command not found' error during package tests.

For more details, see: meteor/meteor#13691
- Removed `sass` as a direct dependency to allow full control over Sass versioning
- Enables usage of both `sass` and `sass-embedded` via devDependencies
- Updated README.md for improved clarity, examples, and migration guidance
- Bumped version to `5.1.0-beta.1` to reflect breaking internal change
@illusionfield
Copy link
Contributor Author

Hi @StorytellerCZ @jankapunkt 👋

I know the previous PR #323 is still pending and this one is not urgent yet — there’s still a long way to go before it would need deeper attention.

That said, I just wanted to check in and ask:
Do you see this as a direction worth continuing, or is this concept something that should rather be dropped?

No pressure at all, I completely understand if priorities are elsewhere. I’d just appreciate any early thoughts before investing more energy into refining this path.

Thanks a lot! 🙏

@StorytellerCZ
Copy link
Member

If anything this would have to be a major version as it requires an action from the package users.

@illusionfield
Copy link
Contributor Author

@StorytellerCZ
Totally agree — this absolutely qualifies as a major version due to the required user action.

I didn’t include it in v5.0.0 originally because that release was already a complete rewrite with a lot of new concepts, and I didn’t want to overload it further.

That said, I’m still waiting for a bit more feedback from you to validate both the direction and the implementation itself.
I don’t want to mess up the v5.0.0 PR — especially since @jankapunkt raised some concerns earlier when the concept was still just theoretical.
It would be really helpful if he could take a quick look now that there’s actual code to review.

If you feel this change belongs in v5.0.0, just let me know and I’ll merge it there. Happy to align with whatever makes the most sense for the package.

@illusionfield
Copy link
Contributor Author

If anything this would have to be a major version as it requires an action from the package users.

Hey,

Do you think it makes sense to roll this change into v5.0.0 as well?
If so, I can reorganize the two PRs and consolidate everything into one. Once that’s done, it would be great to publish a new beta release so the community can start using it and provide feedback — just like with the previous alpha releases, where early input helped fix issues quickly.

Let me know what you think!

@jankapunkt
Copy link
Member

I am pro major versions if things are indeed breaking and I'm ready to review any more PR or publish a new beta/rc

@illusionfield
Copy link
Contributor Author

Great — thanks for confirming!

In that case, I’ll go ahead and rework this PR to target v5.0.0 directly.
The README here was written with v5.0+ in mind, so I’ll revise both the README and the migration guide to reflect a unified and consistent major release.

Do you have any suggestions on how to best structure this so the development history stays clear and old context doesn't create confusion?

Would you prefer that I:

  • close both PRs and open a new consolidated one referencing the old ones, or
  • just close one and update the other?

Also, would a quick cleanup of outdated/inconsistently named branches be helpful at this point?
And should the final PR target master, or would you prefer using a release/5.0.0 branch if you're following a git-flow style?

Lastly, there are a couple of issues that were resolved as part of v5.0.0, but haven’t been closed because I’m not the original author. Happy to help flag or link them if needed.

Let me know what works best for you — I’ll adjust accordingly!

@jankapunkt
Copy link
Member

jankapunkt commented May 12, 2025

Hey @illusionfield I'm blocked this week for something else, hopefully I can get back to this in between things... @StorytellerCZ do you have an opinion on the proposed options?

@StorytellerCZ
Copy link
Member

Please target the release/5.0.0 branch. As for the PRs, which ever works best for you. Link any resolved issues and I will then close them.

@illusionfield
Copy link
Contributor Author

Perfect, thanks!

The two resolved issues are:

I'll go ahead and close the two existing PRs and open a new one targeting the release/5.0.0 branch.
The version in the new PR will be fourseven:scss@5.0.0-beta.1.

I'll link the new PR shortly.

@illusionfield illusionfield deleted the feature/remove-sass branch May 12, 2025 09:01
@illusionfield illusionfield mentioned this pull request May 12, 2025
@illusionfield
Copy link
Contributor Author

Closing this PR in favor of the consolidated version here:
➡️ #329 – Finalize Dart Sass migration & drop direct dependency

That PR targets release/5.0.0 and includes all changes from this one, along with updated docs and migration instructions.
Looking forward to your feedback there!

@illusionfield illusionfield restored the feature/remove-sass branch October 31, 2025 00:27
@illusionfield illusionfield deleted the feature/remove-sass branch December 15, 2025 14:42
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.

3 participants