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 implicit text-only for script and style #1036

Merged
merged 3 commits into from May 25, 2013

Conversation

Projects
None yet
10 participants
@ForbesLindesay
Member

ForbesLindesay commented May 24, 2013

closes #875

If we can remove this feature (probably in 0.32.0) then it will simplify a lot of code and significantly reduce the amount you have to learn when you're new to node.

The first step to removing it is to deprecate it and discourage people from using it, so this pull request logs a warning whenever someone relies on this feature. Dealing with the fallout from this will also help to gauge how many people we're going to annoy by removing this feature.

Crucially, once we actually remove the feature altogether, we will be able to write templates that do things like:

script
  import foo.js

which would be enormously useful.

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay May 24, 2013

Member

I will merge this if there are no complaints by 2013-05-25T12:00:00Z

I realize this will be a pain for some people, I'll see about creating an automated tool to help fix this for people.

Member

ForbesLindesay commented May 24, 2013

I will merge this if there are no complaints by 2013-05-25T12:00:00Z

I realize this will be a pain for some people, I'll see about creating an automated tool to help fix this for people.

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay May 24, 2013

Member

We can reduce pain now by encouraging people to use:

npm install fix-jade -g
fix-jade

which will automatically make the necessary change for all jade files in a directory (recursively).

Member

ForbesLindesay commented May 24, 2013

We can reduce pain now by encouraging people to use:

npm install fix-jade -g
fix-jade

which will automatically make the necessary change for all jade files in a directory (recursively).

@vendethiel

This comment has been minimized.

Show comment
Hide comment
@vendethiel

vendethiel May 24, 2013

Contributor

thanks for what you do :-)

Contributor

vendethiel commented May 24, 2013

thanks for what you do :-)

@stuartpb

This comment has been minimized.

Show comment
Hide comment
@stuartpb

stuartpb May 25, 2013

Contributor

I think this change should be accompanied by a bot that files a pull request on all Node projects using Jade with the fix-jade output, like the bot somebody wrote to update all projects using the 'sys' module when it changed to 'util' in node 0.3.0 (praised at a Seattle Node meetup a few months back with "now THAT is how you introduce breaking changes!")

Granted that was 2.5 years and a few orders of magnitude of Node projects on Github ago, so it may not be as feasible now as it was then.

Contributor

stuartpb commented May 25, 2013

I think this change should be accompanied by a bot that files a pull request on all Node projects using Jade with the fix-jade output, like the bot somebody wrote to update all projects using the 'sys' module when it changed to 'util' in node 0.3.0 (praised at a Seattle Node meetup a few months back with "now THAT is how you introduce breaking changes!")

Granted that was 2.5 years and a few orders of magnitude of Node projects on Github ago, so it may not be as feasible now as it was then.

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay May 25, 2013

Member

@stuartpb that would be awesome, the question is how do we find them? I don't think the GitHub search API is sufficient. If people want to submit pull requests into fix-jade then hopefully we can start running it as a bot to submit pull requests. Unfortunately even once you have a repository there's a fair bit of work to be done in order to make the pull requests with the GitHub API.

I'm going to merge this now. We can encourage people to run fix-jade themselves, which is not very time consuming to do, and if it seems to be a problem we can work on creating a bot.

Member

ForbesLindesay commented May 25, 2013

@stuartpb that would be awesome, the question is how do we find them? I don't think the GitHub search API is sufficient. If people want to submit pull requests into fix-jade then hopefully we can start running it as a bot to submit pull requests. Unfortunately even once you have a repository there's a fair bit of work to be done in order to make the pull requests with the GitHub API.

I'm going to merge this now. We can encourage people to run fix-jade themselves, which is not very time consuming to do, and if it seems to be a problem we can work on creating a bot.

ForbesLindesay added a commit that referenced this pull request May 25, 2013

Merge pull request #1036 from visionmedia/deprecate/implicit-text-only
Deprecate implicit text-only for script and style

@ForbesLindesay ForbesLindesay merged commit 63c7397 into master May 25, 2013

1 check passed

default The Travis CI build passed
Details

@ForbesLindesay ForbesLindesay deleted the deprecate/implicit-text-only branch May 25, 2013

@stuartpb

This comment has been minimized.

Show comment
Hide comment
@stuartpb

stuartpb May 26, 2013

Contributor

@ForbesLindesay I don't know how, but I do know it's been done before (the guy who wrote the bot was at the aforementioned Seattle Node meetup: unfortunately, I didn't catch his name). Step one to figuring out how such a thing would be accomplished would probably be finding one of said bot's pull requests and finding the approach / author from there.

Contributor

stuartpb commented May 26, 2013

@ForbesLindesay I don't know how, but I do know it's been done before (the guy who wrote the bot was at the aforementioned Seattle Node meetup: unfortunately, I didn't catch his name). Step one to figuring out how such a thing would be accomplished would probably be finding one of said bot's pull requests and finding the approach / author from there.

@stuartpb

This comment has been minimized.

Show comment
Hide comment
@stuartpb

stuartpb May 26, 2013

Contributor

Also, how isn't the Github search API sufficient? https://github.com/search?q=extension%3Ajade

Contributor

stuartpb commented May 26, 2013

Also, how isn't the Github search API sufficient? https://github.com/search?q=extension%3Ajade

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay May 26, 2013

Member

The problem with that is that it only provides the first 100 pages, and there's no way of getting more. That means we'd only get coverage of about 10% of all repos with jade files in them.

I've now written some code that retrieves a list of all repositories that are owned by someone who's starred jade. That seems like a good start.

Member

ForbesLindesay commented May 26, 2013

The problem with that is that it only provides the first 100 pages, and there's no way of getting more. That means we'd only get coverage of about 10% of all repos with jade files in them.

I've now written some code that retrieves a list of all repositories that are owned by someone who's starred jade. That seems like a good start.

@stuartpb

This comment has been minimized.

Show comment
Hide comment
@stuartpb

stuartpb May 26, 2013

Contributor

Argh! Github's public-facing search API has different capabilities than their search page? That's regrettable. I'll see what can be done wrt that.

In the meantime I'd also add all the repositories of modules on npmjs.org that list jade as a dependency (if that isn't a strict subset of repos by users starring jade).

Contributor

stuartpb commented May 26, 2013

Argh! Github's public-facing search API has different capabilities than their search page? That's regrettable. I'll see what can be done wrt that.

In the meantime I'd also add all the repositories of modules on npmjs.org that list jade as a dependency (if that isn't a strict subset of repos by users starring jade).

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay May 26, 2013

Member

That's not a bad suggestion, but most of the users of jade probably aren't in npm anyway as they will be the final finished applications.

It's not that their search API provides any less functionality than their search page. It's that neither provides the ability to run a search with more than 100 pages of results.

Member

ForbesLindesay commented May 26, 2013

That's not a bad suggestion, but most of the users of jade probably aren't in npm anyway as they will be the final finished applications.

It's not that their search API provides any less functionality than their search page. It's that neither provides the ability to run a search with more than 100 pages of results.

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay
Member

ForbesLindesay commented May 26, 2013

@jade-bot is go!!!

@elisee

This comment has been minimized.

Show comment
Hide comment
@elisee

elisee Jun 4, 2013

What's the proper syntax for updating script!= javascript? Is it script!= javascript.?

I tried it because that's what the jade-bot offered to merge into express-expose but I get a non-sensical error when parsing my Jade template on the last line of my template:

 Line 39: Unexpected token )
  at throwError (...\node_modules\jade\node_modules\with\node_modules\lexical-scope\node_modules\astw\node_modules\esprima\esprima.js:1156:21)

Is jade-bot/express-expose@6b41eda wrong or am I hitting an unrelated bug?

elisee commented Jun 4, 2013

What's the proper syntax for updating script!= javascript? Is it script!= javascript.?

I tried it because that's what the jade-bot offered to merge into express-expose but I get a non-sensical error when parsing my Jade template on the last line of my template:

 Line 39: Unexpected token )
  at throwError (...\node_modules\jade\node_modules\with\node_modules\lexical-scope\node_modules\astw\node_modules\esprima\esprima.js:1156:21)

Is jade-bot/express-expose@6b41eda wrong or am I hitting an unrelated bug?

@vendethiel

This comment has been minimized.

Show comment
Hide comment
@vendethiel

vendethiel Jun 4, 2013

Contributor

it's wrong

Contributor

vendethiel commented Jun 4, 2013

it's wrong

@stuartpb

This comment has been minimized.

Show comment
Hide comment
@stuartpb

stuartpb Jun 4, 2013

Contributor

I think the bot is wrong in that script!= javascript should work without change (since that's how all the other non-text-only tags work, and the only thing this change does is make script and style work like the other tags).

Contributor

stuartpb commented Jun 4, 2013

I think the bot is wrong in that script!= javascript should work without change (since that's how all the other non-text-only tags work, and the only thing this change does is make script and style work like the other tags).

@elisee

This comment has been minimized.

Show comment
Hide comment
@elisee

elisee Jun 4, 2013

@stuartpb I'm still getting a deprecation warning though, so either the warning itself is spurious (which might well be the case) or there is some change to make.

elisee commented Jun 4, 2013

@stuartpb I'm still getting a deprecation warning though, so either the warning itself is spurious (which might well be the case) or there is some change to make.

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Jun 5, 2013

Member

Hi @elisee sorry about this, it's a bug in the current version of jade and it's entirely my fault. We don't have any test cases for:

script foo()

or

script!= code

so I didn't notice the deprecation warning showing up when it shouldn't.

I'll fix it in the next version of Jade but I'm afraid I have exams for the next couple of days. I'll get right on it Friday.

Member

ForbesLindesay commented Jun 5, 2013

Hi @elisee sorry about this, it's a bug in the current version of jade and it's entirely my fault. We don't have any test cases for:

script foo()

or

script!= code

so I didn't notice the deprecation warning showing up when it shouldn't.

I'll fix it in the next version of Jade but I'm afraid I have exams for the next couple of days. I'll get right on it Friday.

@stuartpb

This comment has been minimized.

Show comment
Hide comment
@stuartpb

stuartpb Jun 5, 2013

Contributor

note to self to look into this

Contributor

stuartpb commented on ccb3ca8 Jun 5, 2013

note to self to look into this

@tsgautier

This comment has been minimized.

Show comment
Hide comment
@tsgautier

tsgautier Jun 5, 2013

I'm getting this warning now - but I don't even know in which file is the offending code. It would be helpful if the warning could point out where the deprecated usage exists so I could find and fix it.

tsgautier commented Jun 5, 2013

I'm getting this warning now - but I don't even know in which file is the offending code. It would be helpful if the warning could point out where the deprecated usage exists so I could find and fix it.

@elisee

This comment has been minimized.

Show comment
Hide comment
@elisee

elisee Jun 5, 2013

@ForbesLindesay ok thanks for the details, good to know it's nothing to worry about and that I don't have to make any changes.

elisee commented Jun 5, 2013

@ForbesLindesay ok thanks for the details, good to know it's nothing to worry about and that I don't have to make any changes.

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Jun 5, 2013

Member

@tsgautier

just searching for script or style should find you the files that might have offending tags. Alternatively:

npm install fix-jade -g
fix-jade

will recursively look for and attempt to automatically fix any files with problems in (and list all the files that had problems). I wouldn't recommend using it without version control though in case it gets something wrong.

@elisee

No problem, obviously you would need to fix it if you had something like:

script
  foo()

You would then need to change it to:

script.
  foo()
Member

ForbesLindesay commented Jun 5, 2013

@tsgautier

just searching for script or style should find you the files that might have offending tags. Alternatively:

npm install fix-jade -g
fix-jade

will recursively look for and attempt to automatically fix any files with problems in (and list all the files that had problems). I wouldn't recommend using it without version control though in case it gets something wrong.

@elisee

No problem, obviously you would need to fix it if you had something like:

script
  foo()

You would then need to change it to:

script.
  foo()
@YukonSaint

This comment has been minimized.

Show comment
Hide comment
@YukonSaint

YukonSaint Jun 5, 2013

Can you tell me how to convert the following code:

script(type='text/javascript', src='/assets/js/vendor/underscore-min.js')

Or do I need to convert it at all?

FYI - I realize that "Implicit textOnly for script and style is deprecated. Use script. or style. instead." might make plenty of sense to Jade contributors and experienced Jade developers. But for someone who is fairly new to Jade it's very confusing. One or two quick examples in the release notes would be an enormous help.

YukonSaint commented Jun 5, 2013

Can you tell me how to convert the following code:

script(type='text/javascript', src='/assets/js/vendor/underscore-min.js')

Or do I need to convert it at all?

FYI - I realize that "Implicit textOnly for script and style is deprecated. Use script. or style. instead." might make plenty of sense to Jade contributors and experienced Jade developers. But for someone who is fairly new to Jade it's very confusing. One or two quick examples in the release notes would be an enormous help.

@stuartpb

This comment has been minimized.

Show comment
Hide comment
@stuartpb

stuartpb Jun 6, 2013

Contributor

@mattsavino: You don't need to convert it, and indeed you shouldn't even get a deprecation warning for script tags with src attributes.

Contributor

stuartpb commented Jun 6, 2013

@mattsavino: You don't need to convert it, and indeed you shouldn't even get a deprecation warning for script tags with src attributes.

@stuartpb

This comment has been minimized.

Show comment
Hide comment
@stuartpb

stuartpb Jun 6, 2013

Contributor

As for a different warning for script and src, maybe this:

"(file): lines (first)-(last): Using text content under a (script / style) element without . is deprecated. Use (script. / style.) instead. (script/style) will be changed to interpret content the same as all other tags in Jade 0.32.0."

Contributor

stuartpb commented Jun 6, 2013

As for a different warning for script and src, maybe this:

"(file): lines (first)-(last): Using text content under a (script / style) element without . is deprecated. Use (script. / style.) instead. (script/style) will be changed to interpret content the same as all other tags in Jade 0.32.0."

@stuartpb

This comment has been minimized.

Show comment
Hide comment
@stuartpb

stuartpb Jun 6, 2013

Contributor

Alternately, if the Jade site is beefed up, the change could be explained in detail on a documentation page and the deprecation warning could just be changed to a hyperlink like "See http://jade-lang.org/textonlytags.html".

Contributor

stuartpb commented Jun 6, 2013

Alternately, if the Jade site is beefed up, the change could be explained in detail on a documentation page and the deprecation warning could just be changed to a hyperlink like "See http://jade-lang.org/textonlytags.html".

@stuartpb

This comment has been minimized.

Show comment
Hide comment
@stuartpb

stuartpb Jun 6, 2013

Contributor

I don't have much experience with the Jade parser, but would #1063 tighten up the deprecation context correctly?

I removed the script+src attribute check because that should really have its own warning, since this case is prohibited by the HTML5 spec.

Contributor

stuartpb commented Jun 6, 2013

I don't have much experience with the Jade parser, but would #1063 tighten up the deprecation context correctly?

I removed the script+src attribute check because that should really have its own warning, since this case is prohibited by the HTML5 spec.

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Jun 6, 2013

Member

OK, I like that new suggestion for a beefed up warning message and I'm happy to have that.

I think the beefing up of the jade site might take slightly more time. I don't want to have random pages that are only used as a place to link to from a deprecation warning. Also, urls won't end with file extensions so it would be something like jade0lang.org/textonlytags/

I agree, I was using the src check to try and catch all the scripts that didn't have blocks, I just didn't account for those with inline blocks. Your solution is much neater.

I quite like the idea of rejecting scripts that have bodies and an src attribute as a compilation error at some future point, but it should be left to further down the line.

Member

ForbesLindesay commented Jun 6, 2013

OK, I like that new suggestion for a beefed up warning message and I'm happy to have that.

I think the beefing up of the jade site might take slightly more time. I don't want to have random pages that are only used as a place to link to from a deprecation warning. Also, urls won't end with file extensions so it would be something like jade0lang.org/textonlytags/

I agree, I was using the src check to try and catch all the scripts that didn't have blocks, I just didn't account for those with inline blocks. Your solution is much neater.

I quite like the idea of rejecting scripts that have bodies and an src attribute as a compilation error at some future point, but it should be left to further down the line.

@stuartpb

This comment has been minimized.

Show comment
Hide comment
@stuartpb
Contributor

stuartpb commented Jun 6, 2013

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Jun 8, 2013

Member

tbh the easiest thing is probably just to write a blog post on the subject. I'll sort it out at some point.

Member

ForbesLindesay commented Jun 8, 2013

tbh the easiest thing is probably just to write a blog post on the subject. I'll sort it out at some point.

@stuartpb

This comment has been minimized.

Show comment
Hide comment
@stuartpb

stuartpb Jun 8, 2013

Contributor

That's kind of what I was thinking http://jade-lang.org/breaking-changes would be - like a doc page that lists breaking changes that have been made in Jade, with each change getting its own blog-post-like section.

Contributor

stuartpb commented Jun 8, 2013

That's kind of what I was thinking http://jade-lang.org/breaking-changes would be - like a doc page that lists breaking changes that have been made in Jade, with each change getting its own blog-post-like section.

@stuartpb

This comment has been minimized.

Show comment
Hide comment
@stuartpb

stuartpb Jun 8, 2013

Contributor

Does this change also mean the include filter for .js files that wrap them in <script> tags is going to be dropped?

Contributor

stuartpb commented Jun 8, 2013

Does this change also mean the include filter for .js files that wrap them in <script> tags is going to be dropped?

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Jun 9, 2013

Member

It's not something I plan on doing in the near future, but I do plan on making filters explicit in some way so that it'll be possible to include a file and just get the plain text of a markdown doc or js file or whatever. That will be the next major breaking change.

I think encouraging people then to switch from what would be something like:

include js:foo/bar.js

to something like

script
  include foo/bar.js

may be a good idea. I don't think this would need to include deprecation warngings right away though.

Member

ForbesLindesay commented Jun 9, 2013

It's not something I plan on doing in the near future, but I do plan on making filters explicit in some way so that it'll be possible to include a file and just get the plain text of a markdown doc or js file or whatever. That will be the next major breaking change.

I think encouraging people then to switch from what would be something like:

include js:foo/bar.js

to something like

script
  include foo/bar.js

may be a good idea. I don't think this would need to include deprecation warngings right away though.

@stuartpb

This comment has been minimized.

Show comment
Hide comment
@stuartpb

stuartpb Jun 10, 2013

Contributor

Wouldn't include:js make more sense, since filters have traditionally been colon-prefixed (and filenames can technically have colons in them)?

Contributor

stuartpb commented Jun 10, 2013

Wouldn't include:js make more sense, since filters have traditionally been colon-prefixed (and filenames can technically have colons in them)?

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Jun 10, 2013

Member

Yes, probably, I haven't settled on any final syntax yet. I'll open an issue so it can be discussed before anything is decided.

Member

ForbesLindesay commented Jun 10, 2013

Yes, probably, I haven't settled on any final syntax yet. I'll open an issue so it can be discussed before anything is decided.

@jescalan

This comment has been minimized.

Show comment
Hide comment
@jescalan

jescalan Jun 15, 2013

Contributor

Hey @ForbesLindesay - can we get at least which file/line the error is coming from in the deprecation warning? Just upgraded on a big project and getting shit tons of these warnings, but no idea where to find them or change them. I'm aware of projects like fix-jade, but I'd like to be able to just jump in and make these changes myself.

Contributor

jescalan commented Jun 15, 2013

Hey @ForbesLindesay - can we get at least which file/line the error is coming from in the deprecation warning? Just upgraded on a big project and getting shit tons of these warnings, but no idea where to find them or change them. I'm aware of projects like fix-jade, but I'd like to be able to just jump in and make these changes myself.

@ForbesLindesay

This comment has been minimized.

Show comment
Hide comment
@ForbesLindesay

ForbesLindesay Jun 16, 2013

Member

Yes, patch welcome

Member

ForbesLindesay commented Jun 16, 2013

Yes, patch welcome

jescalan pushed a commit to jescalan/jade that referenced this pull request Jun 16, 2013

piuccio pushed a commit to ariatemplates/ariatemplates that referenced this pull request Jul 5, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment