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

Deprecate stylus #9445

Merged
merged 8 commits into from
Dec 14, 2017
Merged

Deprecate stylus #9445

merged 8 commits into from
Dec 14, 2017

Conversation

hwillson
Copy link
Contributor

@hwillson hwillson commented Dec 6, 2017

Better / more up to date 3rd party stylus packages exist and there isn't really any technical reason why Meteor core needs to include its own stylus package. Since a stylus package can be fully built and managed outside of core, this commit moves the stylus package into deprecated.

This PR also preps the package contents for deprecation if we decide to publish a final version. Publishing a final (deprecated) version probably isn't necessary, but the changes are included just in-case.

Better / more up to date 3rd party stylus packages exist and
there isn't really any technical reason why Meteor core needs
to include its own stylus package. Since a stylus package
can be fully built and managed outside of core, this commit
moves the `stylus` package into `deprecated` (and preps the
package contents for deprecation if we decide to publish a
final version).
@hwillson
Copy link
Contributor Author

hwillson commented Dec 6, 2017

Hmm - failing webapp and appcache tests that both pass locally, and don't have anything to do with these changes. Something is up somewhere, but it doesn't appear to be related to this PR.

@hwillson
Copy link
Contributor Author

hwillson commented Dec 7, 2017

Just to add more weight to the stylus deprecation decision: stylus/stylus#2282 (comment) (a comment from the lead - and only - upstream stylus maintainer saying he's unsure of it's future, doesn't have time to keep it going himself, asking for help, and not really getting any ... and that was in April 🙁)

@hwillson
Copy link
Contributor Author

hwillson commented Dec 7, 2017

I found the reason for the failures. The failing tests have an indirect dependency on at least one .css file being available in the tested bundle, when the console test runner runs. It turns out that a .css file (/packages/local-test_stylus/stylus_tests.styl.css) included in the stylus package was being picked up and used. By deprecating stylus there are no longer any .css files included in the app bundle when the console test runner runs, causing the tests to fail. Interestingly enough - and what made this so difficult to track down - is that when using the web based test runner (ala meteor test-packages) these tests all pass. This is because the web test runner includes additional .css files (for bootstrap and its own custom styles), which are found and used by tests ... 🤦‍♂️

Long story short - fixes are coming that involve adding a test .css file to the test bundle, and will make it very clear that the .css file is needed (and depended on) by the console test runner.

Some of Meteor's package tests require at least one `.css`
file to be available in the tested application bundle
(e.g. "appcache - sections validity" and "webapp -
content-type header"). The inclusion of this file makes
sure that at least one `.css` file can always be found,
when the tests are run.
@hwillson
Copy link
Contributor Author

hwillson commented Dec 8, 2017

I've committed changes to test-in-console to fix the failing tests here, but those changes aren't really specific to this PR (they should be done regardless). Let me know if they should be reverted here and opened in a new PR. Thanks!

@@ -0,0 +1,8 @@
Package.describe({
summary: 'DEPRECATED - Expressive, dynamic, robust CSS',
version: "2.513.14"
Copy link
Contributor

Choose a reason for hiding this comment

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

If we bump the minor to, say, 2.999.0, it could prevent (some, post-1.4.x) users from automatically updating to the deprecated version (and instead, leave them on the prior 2.513.13) when running meteor update --all-packages (or meteor update stylus). Then, since it wouldn't be "core" in future published Meteor versions, I believe they'd be allowed to keep using it @2.513.13 after that. Am I mistaken?

Otherwise LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense to me @abernix - changes coming - thanks!

Putting the minor version at something unreachable to
make sure the deprecated version isn't accidientally
pulled into an app when running
`meteor update --all-packages` or `meteor update stylus`.
@benjamn benjamn added this to the Release 1.6.1 milestone Dec 13, 2017
@benjamn benjamn merged commit d644705 into meteor:devel Dec 14, 2017
@alexnault
Copy link

Better / more up to date 3rd party stylus packages exist [...]

@hwillson Do you have any recommendations? mquandalle:stylus and stolinski:stylus-multi seem to be the most popular alternatives but they were last updated 2-3 years ago.

@hwillson
Copy link
Contributor Author

@ANault Try coagmano:stylus (it's essentially an up to date version of mquandalle:stylus).

benjamn added a commit that referenced this pull request Jan 22, 2018
This code was recently removed in PR #9445, with this commit:
d644705

Rather than removing deprecated code entirely from the codebase, I think
it's sufficient to keep it in packages/deprecated, and print a deprecation
notice whenever the package is used. This way it's clear that developers
should migrate to other similar packages, but we can still release
important patches for those who haven't been able to migrate yet.

cc @hwillson @abernix
rootedsoftware added a commit to rootedsoftware/meteor-tutorials that referenced this pull request May 30, 2018
skirunman added a commit to skirunman/guide that referenced this pull request Oct 15, 2018
Stylus is no longer supported in Meteor, and was never that popular, so remove from guide.  meteor/meteor#9445
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.

None yet

4 participants