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

Doctype shortcut deprecation patch #1374

Closed

Conversation

sintaxi
Copy link

@sintaxi sintaxi commented Jan 7, 2014

Changes

This patch does two things:

  1. restores the !!! doctype shortcut and doctype 5 modifier
  2. adds deprecation warnings for both.

Rationale

Given how prevalent the use of !!! is, it would be helpful to have a buffer for informing people of this change, and updating applications before being permanently deprecated. Even then, a silent error seems more appropriate than throwing an error to stay consistent with the rest of jades templating behaviour.

Please consider accepting this patch and tagging a v1.0.3 release. It will assist in upgrading to Jade v1 in sintaxi/harp.

all tests are passing

thanks,
Brock

@buschtoens
Copy link

We already discussed that in #1270.

It does not help. People who use "jade": "*" won't listen to console.warn either.

@sintaxi
Copy link
Author

sintaxi commented Jan 7, 2014

It does not help. People who use "jade": "*" won't listen to console.warn either.

@silvinci Is there a specific reason this can't be merged? It helps us, and it's low risk to keep the functionality.

Please see this blog post released on Oct 21st after we upgraded to jade v0.35.0 in the v0.9.4 release of harp. That version of Jade included the script. change. It was also mentioned in the v0.9.4 release notes. As you an see, it does help, and we do listen.

@buschtoens
Copy link

I did not want to attack you personally. If you feel offended, I am very sorry.
Responsible devs, like you, always read the changelog and use proper semver, however the end-user does not in 90 % of the cases. They tend to use * and call it a day.

If those end-users get a deprecation notice in their console (if they even see it) it's likely they ignore it, as long as everything works how they want it to. By failing and throwing a descriptive error, the users are forced to do the code update. Someday they would be forced anyway, so we can do it now aswell, as we have further breaking changes that require a code update too.

Merging this would allow those users to go "I guess I'll do it sometime later... Or never at all". If we thn remove this patch in a later version, we have the same problem we now have.

Better do it once and correct.

I guess this is what @ForbesLindesay had in mind, when he told me that a deprecation notice is no use to us.

@buschtoens
Copy link

For your project's misery, I suggest to hold back the Jade update for one release and implement a warning yourself.

@matejkramny
Copy link

well phrased :P

Interesting debate about semver dependency versioning. Personally, I like the latest and greatest, and if a new feature comes out then great (also the bug and security fixes that come with it).
I haven't got time to check for a new version for every dependency I would ever use in the project.. Yeah its good locking the dependencies to prevent stuff breaking bad

I am wondering how you manage them

Matej Kramny

On Tuesday, 7 January 2014 at 16:01, Jan Buschtöns wrote:

For your project's misery, I suggest to hold back the Jade update for one release and implement a warning yourself.


Reply to this email directly or view it on GitHub (#1374 (comment)).

@notslang
Copy link
Contributor

notslang commented Jan 9, 2014

In a case like this, where backwards compatibility is so easy to maintain, it is crazy to break compatibility with the vast majority of existing templates. Especially in a language with documentation as shitty as this.

From Wikipedia

While a deprecated software feature remains in the software its use may raise warning messages recommending alternative practices; deprecated status may also indicate that feature will be removed in the future. Features are deprecated rather than immediately removed to provide backward compatibility and give programmers time to bring affected code into compliance with the new standard.

Also:

however the end-user does not in 90% of the cases.

Where did you get the "90%" statistic @silvinci? Can you actually point out a 90% rate of ignoring deprecation warnings from data that's been collected? Or even a vast majority?

@buschtoens
Copy link

It's the decision made by the maintainer. We tried convincing him to deprecate the feature before removing it, but he argued that this is not the only breaking change that would be needed to push jade forwards. So he led them all up in one version so fixing the templates should be easier and a one-time thing.

Obviously the 90 % was not meant literally. In the past it often was the case that things broke and people complained, although it was deprecated a long time ago.

If you're using jade in a private project (i.e. not an npm module for other users) it shouldn't be a too big deal to update your files. If you have 10+ files containing !!! you might be doing templating wrong.

If you're using jade in a project that is used by others this change does impose a problem. Users who update a meta package wouldn't expect sudden code breakage. Still, fixing the code is easy.

I don't want to justify this change as I had minor problems with it aswell, but I want to make it understandable. The commit was pushed and it won't be reverted.

What is your exact problem? We can try to work out a convenient fix.

@sintaxi
Copy link
Author

sintaxi commented Jan 11, 2014

What is your exact problem? We can try to work out a convenient fix.

@silvinci Our platform has couple thousand applications in production with the previously (documented by jade) way of doing things, if we upgrade to v1 the next time they deploy their app will be broken without any warning. You can help by merging this perfectly valid PR. or at the very least make !!! fail silently.

The actual merits of this PR have yet to be addressed. So far all we've heard from the jade team is "we decided to change it, deal with it.".

I understand things need to break sometimes to move things forward in a positive way but the rationale for not having a deprecation warning for this feature is is basically non-existent "deprecation warnings don't help. people don't pay attention to them" which I demonstrated with facts that prove otherwise in the case when the script. change happened we blogged about it, put it in our release notes, and emailed our users about the change.

If a customer gets upset at us for something breaking it helps a lot to have demonstrated that we put forth every effort to warn them of the change.

Like I say !!! has been in nearly every documented example of jade since the inception of the library. It literally has been the first line of documentation and now it breaks and throws an error. Do you really not see a problem with that?

I think @slang800 sums it up best.

In a case like this, where backwards compatibility is so easy to maintain, it is crazy to break compatibility with the vast majority of existing templates. Especially in a language with documentation as shitty as this.

Anyway, at the end of the day, we can stick with v0.35.0 and start warning our customers and changing our docs before upgrading to v1. Its really not that big of a deal. I'm left a bit concerned however with the lack of empathy being applied to the situation and the thought that its ok to deliberately make the "hello world" example code of jade break without warning. Its a bit more reckless than Im comfortable with for a library that was downloaded 4.5M times in the last 12 months, but if its what has to happen to move the library forward than I guess I can live with it.

With all that said. I'm grateful for this project and the work that is being done to improve jade. Although this is a complaint, please don't be discouraged. I'm overall happy with the work being done here. Cheers.

@buschtoens
Copy link

I can't merge this PR, as I am no enlisted collaborator for this repository. Even if I was, I probably wouldn't. Not because I decided so, but because @ForbesLindesay did so.

He put up these two main reasons:

People don't update their code until it actually breaks. [...] Without breaking things, people won't change them.
I also want to get all the breaking changes into a single release. There are going to be at least 4 major breaking changes in that release, so adding one more is not that expensive.

While people won't update might not be fully and always true, putting multiple breaking changes in one release is better (IMO).

I can understand your reasoning very well and partially feel the same. It really is easy to mantain backwards compatibility in this case. However, a decision was made and I don't expect the devs to revert it, although I'd welcome it.

I can only encourage you to stick with v0.35.0 and inform your customers asap giving them a certain time frame, in which they can update their apps. It's not like your customers are really "missing out" on something, while you hold the update back. Again, I feel very sorry for the situation we've caused you and your team.

@notslang
Copy link
Contributor

@silvinci, just because @ForbesLindesay said something, it isn't necessarily correct. He is a good programmer, and is often right, but you really gotta think for yourself.

One of the main reasons why open source projects like this are so successful is because they benefit from the collective experience of all their contributors. Hundreds of people watching over them and suggesting improvements. Thus, if you blindly follow whoever the maintainer is, you are no longer contributing one of your most valuable assets: your ideas.

@rlidwka
Copy link
Member

rlidwka commented Jan 12, 2014

The change is already in npm registry. It doesn't make sense to remove something, then bring it back only to deprecate it.

Just put "0.35.x" in your package.yaml file where all your dependencies are and get it over with.

@notslang
Copy link
Contributor

To add to my last comment; this actually reminds me of a wonderful passage from the Valve Handbook:

There are lots of stories about how Gabe has made important decisions by himself, e.g., hiring the whole Portal 1 team on the spot after only half of a meeting. Although there are examples, like that one, where this kind of decision making has been successful, it’s not the norm for Valve. If it were, we’d be only as smart as Gabe or management types, and they’d make our important decisions for us. Gabe is the first to say that he can’t be right nearly often enough for us to operate that way. His decisions and requests are subject to just as much scrutiny and skepticism as anyone else’s. (So if he tells you to put a favorite custom knife design into Counter-Strike, you can just say no.)

@ForbesLindesay
Copy link
Member

@slang800 indeed, I'm not always right. Maybe I was wrong in this instance, maybe I wasn't. It is useful to have the collective experiences of the many jade contributors when making development decisions. That is why you will see me making pull requests, rather than commiting to the master branch for almost all changes. If you look back through my pull requests you will see that I typically wait at least 24 hours before merging those pull requests. This is to give other people time to comment/raise objections. If there are objections, I will typically not merge the pull request until after some discussion. If you wanted to argue for gradual deprecation with a warning message, followed by later removal, that was your chance.

It is now too late.

@sintaxi
Copy link
Author

sintaxi commented Jan 12, 2014

I ask honestly, why do you think it is too late?

@buschtoens
Copy link

I guess @rlidwka says it:

The change is already in npm registry. It doesn't make sense to remove something, then bring it back only to deprecate it.

@silentrob
Copy link

It is never to late to fix a mistake.
This change also breaks express generate.

express --css stylus --jade myapp

cd myapp
node app.js

@sintaxi
Copy link
Author

sintaxi commented Jan 13, 2014

This situation reminds me of when the sys alias for util was removed and replaced with an exception in nodejs. The discussion that followed and the conclusion the they came to in the end is pretty interesting. Worth the read.

nodejs/node-v0.x-archive#3577

@buschtoens
Copy link

@sintaxi I was looking for that issue. Thanks!

@sintaxi
Copy link
Author

sintaxi commented Jan 14, 2014

@silvinci np. here is the postmortem from Isaac. The resemblance to this issue is uncanny.

https://groups.google.com/forum/?hl=en?hl%3Den#!msg/nodejs/oqlT9ZtUZd0/XicWJx6mC4oJ

@tj
Copy link
Contributor

tj commented Jan 14, 2014

warn/throw/remove is a pretty good policy, don't always follow it myself but I agree with it ;D

@jonathanong
Copy link
Contributor

Yeah straight up throwing is a little too much, especially without prior warning. That's one reason I added all those warnings to connect. Looks like we have to release a patch in express pretty much just for this since too many people are complaining.

@buschtoens
Copy link

/whisper: @jonathanong (except someone would hit "merge", but that would have other consequences) :D

@ForbesLindesay
Copy link
Member

OK, looks like I should probably have released a version with a warning in. If someone wants to build a version of jade that has warnings, rather than throwing/not working for everything that is deprecated in 1.0.0 then I am prepared to release it as 0.36.0. It isn't too late to do that, but we're not going to have 1.x.y start un-deprecating things. Although this specific change could've been left as backwards compatible, many other breaking changes couldn't have been done in that way.

The list of things you'd need to add deprecation warnings for if you want such a version released is as follows:

  • node@0.8 (although I believe it currently works, it's no longer supported)
  • running on browsers that need the polyfills
  • globals that aren't in the list
  • apostrophes in data-attributes
  • non-literal . vs. literal .
  • parsed comments
  • conditional comments
  • !!! shortcut for doctype
  • 5 shortcut for html
  • colons option (this might already be there)
  • client option
  • self closing tags with content
  • the string #[ which no signifies the start of an inline tag
  • the attributes attribute
  • `tag wrapping for filters (not sure what you can do here tbh)

If it doesn't warn for all of those things, you're not sticking by what you've said should have been done. I don't personally have the time to create such a version right now as it's a large amount of work to write & test, and since all it would lead to is the same complaints because the people who aren't able to cope with locking their version number at 0.35.0 aren't going to manage to lock their version at 0.36.0 either.

Since we were pre 1.0.0, the change was made in accordance with Semantic Versioning, going forwards I am going to be doing my very best to continue to stick to Semantic Versioning rules. In accordance with Semantic Versioning, pre 1.0.0 dependencies should be locked to a specific version.

@ForbesLindesay
Copy link
Member

Additionally, even if I were looking to make this change, the current pull request would not be accepted since it does not add line numbers or file names to the warnings. In this respect, throwing errors is far more helpful.

@stuartpb
Copy link
Contributor

That is why you will see me making pull requests, rather than commiting to the master branch for almost all changes. If you look back through my pull requests you will see that I typically wait at least 24 hours before merging those pull requests. This is to give other people time to comment/raise objections. If there are objections, I will typically not merge the pull request until after some discussion. If you wanted to argue for gradual deprecation with a warning message, followed by later removal, that was your chance. It is now too late.

  • MISTER PROSSER: I'm afraid you're going have to accept it! This bypass has got to be built and it is going to be built. Nothing you can say or do -
  • ARTHUR DENT: Why has it got to be built?
  • MISTER PROSSER: Wha - what do you mean, “why has it got to be built?” It is a bypass! You've got to build bypasses!
  • ARTHUR DENT: Didn't anyone consider the alternatives?
  • MISTER PROSSER: There aren't any alternatives! But you are quite entitled to make any suggestions or protests at the appropriate time!
  • ARTHUR DENT: Appropriate time?
  • MISTER PROSSER: Yes.
  • ARTHUR DENT: The first I knew about it was when a workmen arrived at the door yesterday.
  • MISTER PROSSER: T- oh!
  • ARTHUR DENT: I asked him if he'd come to clean the windows and he said he'd come to demolish the house! He didn't tell me straight away of course. Oh no. First he wiped a couple of windows and charged me a fiver. Then he told me.
  • MISTER PROSSER: But Mister Dent the plans have been available in the planning office for the last nine months!
  • ARTHUR DENT: Yes! I went round to find them yesterday afternoon. You'd hadn't exactly gone out of your way to pull much attention to them have you? I mean, like actually telling anybody or anything.
  • MISTER PROSSER: The plans were on display.
  • ARTHUR DENT: Ah! And how many members of the public are in the habit of casually dropping around the local planning office of an evening?

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.

10 participants