Rebuild hyperlink #2382
Merged
Rebuild hyperlink #2382
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Missing, you mean 404? |
|
Technically it's an |
|
Current status: 46 Ignored Errors151 Errors |
montogeek
reviewed
Jul 25, 2018
| @@ -93,6 +93,6 @@ Use a different service URL in production/development builds: | |||
|
|
|||
| ```javascript | |||
| new webpack.DefinePlugin({ | |||
| 'SERVICE_URL': JSON.stringify('http://dev.example.com') | |||
| 'SERVICE_URL': JSON.stringify('https://dev.example.com') | |||
There was a problem hiding this comment.
Unnecessary, but showing a good example imo
|
Current Status: 70 Todo's129 Errors |
|
JSON loader is included by default since webpack 2, but we are still listing it in our docs. I think we could remove it. |
montogeek
reviewed
Jul 25, 2018
src/utilities/process-readme.js
Outdated
| @@ -1,4 +1,6 @@ | |||
| module.exports = function processREADME(body) { | |||
| const Url = require('url'); | |||
montogeek
reviewed
Jul 25, 2018
| let indexPath = page.type === 'index' ? '/index' : ''; | ||
| let mainPath = page.url.startsWith('/') ? page.url : `/${page.url}`; | ||
| let editLink = page.edit || baseURL + TrimEnd(mainPath, '/') + indexPath + '.md'; | ||
| const editLink = page.edit || Url.resolve(baseURL, page.path); |
montogeek
reviewed
Jul 25, 2018
| @@ -3,6 +3,10 @@ | |||
|
|
|||
| .organization { | |||
| padding: 1.5em; | |||
|
|
|||
| &__title > h4 { | |||
| margin-top: 0; // Overrides markdown scss | |||
There was a problem hiding this comment.
Why add markdown in the first place?
There was a problem hiding this comment.
Seems all the basic styles are now scoped under .markdownwhere they previously existed under page__content (now removed).
I'm not happy with this solution, but I'm not going to redo the CSS architecture as well when my focus is fixing broken links
montogeek
reviewed
Jul 25, 2018
| @@ -69,6 +71,9 @@ class Site extends React.Component { | |||
| render={props => ( | |||
| <Container className="site__content"> | |||
| <Switch> | |||
| <Route path="/vote" component={Vote} /> | |||
montogeek
reviewed
Jul 25, 2018
| @@ -24,7 +24,7 @@ if (isClient) { | |||
| let { pathname } = window.location; | |||
| let trimmed = pathname.replace(/(.+)\/$/, '$1'); | |||
| let entryPage = findInContent(Content, item => item.url === trimmed); | |||
| let entryPath = entryPage.path.replace('src/content/', ''); | |||
| let entryPath = entryPage && entryPage.path.replace('src/content/', ''); | |||
There was a problem hiding this comment.
/vote, /organization and starter-kits are not in the content tree based on discovering markdown files in the file system. This causes entryPage to be undefined when visiting any of those pages, which then throws on undefined.path.replace()
montogeek
reviewed
Jul 25, 2018
| @@ -12,7 +12,12 @@ const contentTree = require('./src/_content.json'); | |||
| const common = require('./webpack.common.js'); | |||
|
|
|||
| // content tree to path array | |||
| const paths = flattenContentTree(contentTree); | |||
| const paths = [ | |||
|
@Munter Have you found issues regarding OpenCollective urls? |
|
Current status: |
|
All linting except proselint for some externally fetched files is passing. I think this branch is ready to merge into I haven't set up link checking in any CI setup. The reason for that is that there are still some false negatives it is reporting, which might require a roundtrip or two with improvements in hyperlink. In general link checking is quite brittle and should probably never be a blocking check. So for now, lets get it merged and do some manually checks once in a while until we figure out a way to integrate into CI that doesn't annoy and block people |
|
How much work the hyperlink would be to make it more reliable? |
|
Please chime in @jeremenichelli @EugeneHlushko |
|
It's really hard to tell. Inherently this project is unstable because it pulls in so many external pieces of documentation, which might introduce errors. Also networks might fail, services might disappear etc. So it's really not a stable thing in general to check external links. I've ben thinking of adding an internal link check only. That feature doesn't exist yet. That would be fair to block on if there are failures there. But in the mean time, all the content and build changes I have made here are good and improve the quality immensely. Also, there is now a script that people can call to run the link checker themselves. All improvements over the previous status |
skipjack
approved these changes
Jul 31, 2018
There was a problem hiding this comment.
This looks good to me, and I'm happy we're close to getting this re-integrated. I still don't have much free time on my hands but I tested this out as well as the rebuild branch and both seemed to work fine (at least for the main npm scripts I ran).
npm run linkchecks only returned 5 failed errors, so once those are addressed this can be merged as far as I'm concerned but I'll leave it to the new maintainers (i.e. @montogeek @EugeneHlushko and @jeremenichelli).
The 5 failed were:
✖ FAIL fragment-check dist/api/loaders/index.html --> https://github.com/webpack/loader-utils#getoptions
| operator: fragment-check
| expected: id="getoptions"
| at: dist/api/loaders/index.html:119:21 <a href="https://github.com/webpack/loader-utils#getoptions">...</a>
✖ FAIL fragment-check dist/migrate/3/index.html --> https://github.com/webpack/loader-utils#parsequery
| operator: fragment-check
| expected: id="parsequery"
| at: dist/migrate/3/index.html:239:291 <a href="https://github.com/webpack/loader-utils#parsequery">...</a>
✖ FAIL fragment-check dist/api/compiler-hooks/index.html --> https://github.com/webpack/tapable#tapable
| operator: fragment-check
| expected: id="tapable"
| at: dist/api/compiler-hooks/index.html:24:52 <a href="https://github.com/webpack/tapable#tapable">...</a>
✖ FAIL external-check https://badge.fury.io/js/closure-webpack-plugin
| operator: external-check
| expected: 200 https://badge.fury.io/js/closure-webpack-plugin
| actual: 500 https://badge.fury.io/js/closure-webpack-plugin
| at: dist/plugins/closure-webpack-plugin/index.html:1:50816 <a href="https://badge.fury.io/js/closure-webpack-plugin">...</a>
✖ FAIL external-check https://badge.fury.io/js/polymer-webpack-loader
| operator: external-check
| expected: 200 https://badge.fury.io/js/polymer-webpack-loader
| actual: 500 https://badge.fury.io/js/polymer-webpack-loader
| at: dist/loaders/polymer-webpack-loader/index.html:1:69618 <a href="https://badge.fury.io/js/polymer-webpack-loader">...</a>
|
The 3 first of those words are caused by GitHub not following the HTML standard way of having ID's for fragments. They do some non-standard JavaScript handling of fragment navigation, which hyperlink doesn't have specifically. It might in the future. I still think we should merge. On this branch the linkcheck script is not in the CI scripts because of the unstable nature of testing uncontrollable external pages. But on the rebuild-travis-stages branch I created a setup where only internal doc looks are checked with hyperlink in a blocking manner and the external check runs as non-blocking. So that branch complements this one quite well |
|
@jeremenichelli @EugeneHlushko Any feedback? I would like to merge this one. |
montogeek
approved these changes
Jul 31, 2018
* Fix internal fragmtn links in optimization.md * Skip link checking or opencollective.com/webpack. Massive html response made the checker go into CPU overdrive * Try out travis staged build * Add proselint * Upgrade pip before using it * Move before-hooks around to try and make proselint install * Try adding python as a language as well to geet an updated version * More messing with config * Manually handle node versioning * Add node minor version to nvm install. Defaulted to slightly too low version * Manually install node 8.11 * Try a matrix build to separate node and python stuff * Add linkcheck task and try a different cahce setup * Minor name change to test if cache works correctly across builds * Attempt to combine matrix and staged build * Attempt going back to staged build * Bump travis * Ping Travis. You alive? * Fix broken travis.yml * Fix wrong stage order * Explicitly run specific lintings, exclude proselint * Allow failures for link checker * Change proselint cache directory. Maybe this works * Add new script to fetch repository names that the docs should contain. Try to centralize github API usage to avoid rate limitations * Add caching to eslint * Remove parts of deploy.sh that are now run by travis * Added new ./repositories to store github api results * Replace fetch.sh with npm scripts and fetch-package-readmes.js * Attempt to make caches more specific to the containers and stages they refer to * Remove deprecaed fetch_packages script. Now replaced by two-step fetch-package-repos and fetch-package-readmes * Attempt a more windows friendly build by using npm and webpack instead of shell commands * Disable link checking for now to speed up builds * Revert "Disable link checking for now to speed up builds" This reverts commit 7d4bb06. * Add dist to proselint cache so generated content also gets checked * Remove unnessessary GITHUB_TOKEN env variable check in repo fetching script * Revert "Add dist to proselint cache so generated content also gets checked" This reverts commit fc7676d. * Rework pipeline for better speed and make proselint a deployment blocker * Rename build stage to reflect its actions * Run content-tree only after generating all external content * Remove link to non-existing named-modules-plugin * Fix double slashes in edit links * Rename stages * Add new internal link check as a blocking check * Fix wrong name in allowed_failures config
montogeek
reviewed
Aug 2, 2018
| directories: | ||
| - $HOME/.cache | ||
| install: pip install -r requirements.txt | ||
| script: cp .proselintrc ~/ && proselint src/content |
There was a problem hiding this comment.
Is this really running before build?
There was a problem hiding this comment.
There was a problem hiding this comment.
Proselint is running in parallel with the remaining linting and the build. This is because it tends to be a slow run, but a really fast container boot because it only relies on the python ecosystem.
Proselint and the remaining limiting plus build run in parallel. If any of them fails it fails the pipeline, so you'll have the same effect as before, but faster once you remove the parallelization limitation
montogeek
reviewed
Aug 2, 2018
| - yarn lint:markdown | ||
| - yarn lint:social | ||
| - yarn build | ||
| - yarn lint:links |
There was a problem hiding this comment.
yarn lint:links is failing too https://travis-ci.org/webpack/webpack.js.org/jobs/411353517
There was a problem hiding this comment.
Random badge timeout. They seem to happen a lot. Might want to mark those as optional with a --todo
montogeek
reviewed
Aug 2, 2018
| "lint:markdown": "markdownlint --config ./.markdownlint.json *.md ./src/content/**/*.md --ignore './src/content/**/_*.md'", | ||
| "lint:social": "alex . -q", | ||
| "lint:prose": "cp .proselintrc ~/ && proselint src/content", | ||
| "lint:links": "hyperlink -r dist/index.html --canonicalroot https://webpack.js.org/ -i | tee internal-links.tap | tap-spot", |
There was a problem hiding this comment.
What is tee internal-links.tap?
There was a problem hiding this comment.
Tee writes a copy of stdout to a file while paying through the stdout pipe. It let's us keep a copy of the raw tap output while seeing the nice formatted report in the console. It's just in case we need to go back and look at the output in a different manner, for example if the reporter has a bug, or we forgot to copy/paste the console output
There was a problem hiding this comment.
It is a dependency or something available in linux?
There was a problem hiding this comment.
Oh yeah, linux program, cool! TIL
|
This seems good to go |
|
@Munter Shouldn't we upgrade to webpack 4.x for the project, what do you think? |
|
@Legends We already talked about it 4 months ago, we will do it after merging rebuild. Merging it is top priority. |
|
@Legends Do you want to do it after merging it? |
|
@montogeek We talked about it? I really can't remember 😄. |
|
Thank you so much! |
montogeek
reviewed
Aug 17, 2018
| @@ -8,6 +10,16 @@ module.exports = function processREADME(body) { | |||
| .replace(/<h1.*?>.+?<\/h1>/, '') | |||
| .replace(/# .+/, '') | |||
| .replace(/.*\n=+/, '') | |||
| // Replace local github links with absolute links to the github location | |||
There was a problem hiding this comment.
This change broke some readmes:
From: https://raw.githubusercontent.com/webpack-contrib/polymer-webpack-loader/master/README.md
Original links:
[](https://badge.fury.io/js/polymer-webpack-loader)
[](https://travis-ci.org/webpack-contrib/polymer-webpack-loader)
> [Polymer](https://www.polymer-project.org/) component loader for [webpack](https://webpack.js.org/).After processed by this line:
undefined](https://badge.fury.io/js/polymer-webpack-loader)
undefined](https://travis-ci.org/webpack-contrib/polymer-webpack-loader)
> undefined component loader for undefined.@Munter Could you please take a look, we could maybe avoid that transformation at all
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces #1582
Replaces link checking with updated version of hyperlink.
There is a number of content issues that are being revealed when running the link checking. I'll do my best to create an updated checklist of known issues here.
Ping @montogeek @skipjack @pierreneter @papandreou (involved in old PR)
distrather than the original package github repository's url/assets/in/dist(referenced in a few places, like branding.md)/voteis missing/organizationis missing/starter-kitsis missing/plugins/mini-css-extract-pluginis missing/plugins/named-modules-pluginis missingedit-link on pages not resolving correctly to a github edit url