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

Newlines within <pre> tags are removed #809

Closed
JayBerlin opened this issue Oct 29, 2019 · 12 comments
Closed

Newlines within <pre> tags are removed #809

JayBerlin opened this issue Oct 29, 2019 · 12 comments
Labels
bug fixed on dev for issues fixed on dev branch

Comments

@JayBerlin
Copy link

JayBerlin commented Oct 29, 2019

Describe the bug

If you use a multiline @apiDescription that contains a code block, the newlines within the code block are removed, resulting in an incorrect Description.

Steps to reproduce

Create a comment that contains:

@apIDescription Extended usage of @apiexample with different example types.
Multiline description with pre tags within it

    with multiline code
    second line of code

And more lines at end

Information

The above description results in HTML that looks like this:

<p>Extended usage of @apiExample with different example types. Multiline description with pre tags within it</p> <pre><code>\twith multiline code \tsecond line of code </code></pre> <p>And more lines at end</p>

As you can see, the \n's are lost in the <pre> tag

Fix

I have a fix for this, however, I do not understand how to update the tests to prove the fix and generate a PR.

To fix,
In lib/parser.js (in apidoc-core):

-           // remove line breaks
-           value = value.replace(/(\r\n|\n|\r)/g, ' ');
+          // remove line breaks, but not within <pre> tags
+          value = value.replace(/(?:^|<\/pre>)[^]*?(?:<pre>|$)/g, function(m) {
+              return m.replace(/(\r\n|\n|\r)/g, ' ');
+          });

A similar change needs to be made in the template/utils/handlebars_helper.js

-        return ('' + text).replace(/([^>\r\n]?)(\r\n|\n\r|\r|\n)/g, '$1' + '<br>' + '$2');
+        return ('' + text).replace(/(?:^|<\/pre>)[^]*?(?:<pre>|$)/g, function(m) {
-           return m.replace(/[\n\t]+/g, "");
+           return m.replace(/([^>\r\n]?)(\r\n|\n\r|\r|\n)/g, '$1' + '<br>' + '$2');
+       });
     }
@JayBerlin
Copy link
Author

I am not sure this is a question as it seems to be a pretty clear bug. As I said, I would provide a PR if someone can explain how to update the 3 impacted repos at the same time.

@NicolasCARPi
Copy link
Collaborator

You'll need to do 3 prs. I have write access on the 3 so I can merge the 3 at the same time ;)

@JayBerlin
Copy link
Author

Is there a reason that the project is in 3 repos? That makes it pretty hard for others to contribute.

@NicolasCARPi
Copy link
Collaborator

The idea was to separate the core so that it can be used by other projects AFAIK.

@JayBerlin
Copy link
Author

Sorry, but I do not quite understand how I can update the apidoc-example repo to provide a test for apidoc-core and get my changes included as part of apidoc-core so that I can update those tests since the apidoc-example is included as a dependency for apidoc-core. Same for apidoc.

@NicolasCARPi
Copy link
Collaborator

IMHO the apidoc-example should be nuked, with examples in the examples folder of the main repo, and tests in the test folder. I'm not sure if the apidoc-example repository is for examples or tests, and if it's tests, it makes no sense to have it outside of the main source code it's testing.

On the core, I can see how splitting the core parser from the templating lib could be a good thing. While it makes things a bit more difficult to edit (having PRs depending on the merge in another repo), in theory it results in a cleaner and sane codebase where all the parsing stuff is done by one thing, and the templating in another.

One option would be to make the core repo read only (archive it), and add back the core in this repo, but I'm not sure if it would be a good idea. Also, 40 projects depend on the core package (granted, most of them haven't been updated in a while).

@JayBerlin
Copy link
Author

That makes sense to remove the example project from the repos as a dependency. The apidoc/apidoc-core split makes more sense, and is a little more easily managed.

@rottmann
Copy link
Member

@NicolasCARPi the name apidoc-example wasn't chosen well. I only wanna save some work to duplicate some tests.

apidoc-example should be better removed and necessary core-tests should go to the apidoc-core and frontend-tests-only to apidoc. (clean separation)

In the past some company used apidoc-core in their projects, without the frontend, so it make sense to keep frontend and backend divided.

The frontend was only meant to be a kind of theme, which everybody could replace with their own.

@NicolasCARPi
Copy link
Collaborator

@JayBerlin What I did is:

  • git clone apidoc
  • npm install
  • git clone apidoc-core
  • npm install
  • in apidoc, rm -r the node_modules/apidoc-core and make a symbolic link to the repository cloned folder.

Then you can make two PRs.

But I agree that the fact that one needs to update two different repos for a single change shows that the split might not be the best.

@NicolasCARPi
Copy link
Collaborator

That makes sense to remove the example project from the repos as a dependency.

Oh I didn't realize that the apidoc-example was a dependency!!! Let's deprecate this repo and I'll remove it from the devDependencies. Too much repos kills the repos :)

NicolasCARPi added a commit to apidoc/apidoc-core that referenced this issue Nov 22, 2019
@simondotm
Copy link

simondotm commented Apr 18, 2021

I've found that as a workaround, the <pre> section in the @apiDescription can generate newlines using <br>. This is possibly exploiting another existing bug, since I'd assume that any html tags in the content of a <pre> block should be inlined rather than parsed as html.

/**
 * ....
 * @apiDescription
 * ```
 * Line1<br>
 * Line2<br>
 * ```
 */

Results in

Line1
Line2

Rather than

Line1 Line2

Although we would have expected:

Line1<br>
Line2<br>

@NicolasCARPi NicolasCARPi added fixed on dev for issues fixed on dev branch and removed question labels Sep 6, 2021
@NicolasCARPi
Copy link
Collaborator

Hello @JayBerlin,

I have implemented your fix, thank you, it's much better when users report a bug including a fix ;)

Also, currently on the dev branch I have addressed all of the shortcomings discussed above, you don't need 3 PR on 3 separate repositories anymore to make a change. Core has been merged and tests are using the example folder.

Cheers,
~Nico

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fixed on dev for issues fixed on dev branch
Projects
None yet
Development

No branches or pull requests

4 participants