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

Fix building documentation on Windows #8364

Merged
merged 3 commits into from
Nov 5, 2020
Merged

Fix building documentation on Windows #8364

merged 3 commits into from
Nov 5, 2020

Conversation

psmyrek
Copy link
Contributor

@psmyrek psmyrek commented Oct 27, 2020

Suggested merge commit message (convention)

Fix: Fixed building documentation on Windows. Closes #7212.


Additional information

This PR is the 1 of 2 PRs that are needed to fix building documentation on Windows - the second one is https://github.com/cksource/umberto/pull/884.

TL;DR

It works on Linux and on Windows now! 🎉

Details

The main problem was with mixed / and \, which were used in HTML links and paths on filesystem. After some investigation it turned out, that:

  • we can't use methods from built-in path module, because they always produce paths with native separator,
  • we can't use POSIX versions of methods from built-in path module (e.g. path.posix.<methodName>), because of their unreliability and inconsistency.

A lot of parts of code implicitly expect that the one and only valid path separator is /, so I've introduced path module replacement - the upath module - which unifies all paths to the same format, always! This solution works on Windows, because Windows accepts either own \, or / as path separator. One exception to this rule is that webpack also accepts both separators and automatically fixes inconsistencies, but only for the entry option. All other configuration options require native path separators. 😒

If you previously have built docs on Linux and now you want to check it on Windows, then...

  • it will throw permission error, because Windows' version of fs.readdirSync() (used internally by the glob module) is not able to handle build/docs/ckeditor5/latest link created previously on Linux. To continue, simply remove this link manually (or the whole build folder).
  • probably you will need node-sass binary for Windows. Instead of re-installing all dependencies, it is sufficient to just run npm rebuild node-sass and then just start yarn docs again.

To avoid such annoyances, simply choose one OS and stick to it, always.

@mlewand mlewand self-requested a review November 3, 2020 09:15
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

Cool, looks very, very promising!

I have noticed following things:

Stylesheets in our guide have wrong path separator (should be unix style). Open build/docs/ckeditor5/23.1.0/examples/builds/balloon-block-editor.html file, you'll see:

    <link rel="stylesheet" href="../../../../assets/1.6.0/css/styles.css" type="text/css" media="all">
    <link rel="stylesheet" href="../../assets/styles.css" type="text/css">
    <link rel="stylesheet" href="..\..\snippets\examples\balloon-block-editor\snippet.css" type="text/css" data-cke="true">
    <link rel="stylesheet" href="..\..\assets\snippet-styles.css" type="text/css" data-cke="true">
    <script>
      ( function() {

Also some scripts have wrong separator (example form build/docs/ckeditor5/23.1.0/examples/builds/custom-build.html):

</script>
    <script src="..\..\assets\snippet.js"></script>
    <script src="..\..\snippets\examples\custom-build\snippet.js"></script>
  </body>

The order of items in build/docs/ckeditor5/23.1.0/api.json file is different, but the items content seem to match. So I don't think this is a problem as the file is a single array anyway.

All in all the only thing that we're missing are the paths and we're good to go, nice one! 🙂

@psmyrek
Copy link
Contributor Author

psmyrek commented Nov 4, 2020

Paths for styles and scripts have been unified to always be Unix style.

@mlewand mlewand self-requested a review November 5, 2020 08:54
Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

Nice! It works fine 👌

@mlewand mlewand merged commit c7f69dd into master Nov 5, 2020
@mlewand mlewand deleted the i/7212 branch November 5, 2020 08:55
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.

Building docs doesn't work on windows
2 participants