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

chore: Docusaurus with webpack 5 #500

Closed
wants to merge 5 commits into from

Conversation

slorber
Copy link
Contributor

@slorber slorber commented Apr 23, 2021

Use an unreleased version of Docusaurus to test the webpack5 support works for this site, detect potential problems, and perf improvements.

See also facebook/docusaurus#4089

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Apr 23, 2021
@slorber slorber marked this pull request as ready for review April 30, 2021 17:46
@facebook-github-bot
Copy link
Contributor

@Huxpro has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@slorber has updated the pull request. You must reimport the pull request before landing.

@slorber
Copy link
Contributor Author

slorber commented May 1, 2021

ready for review, let me know if anything does not work

Copy link
Contributor

@Huxpro Huxpro left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! Left some questions so I can better understand these changes :)

@@ -39,4 +39,4 @@ This process is currently happened in different places for different platforms:

1. For Android, this happened in `hermes/android/build.gradle`
2. For Apple platforms, this happend in `hermes/utils/build-apple-framework.sh`
3. For Emscripten, you can find an example from the `test-emscripte` job from `hermes/.circleci/config.yml`. Also see more details at [Building with Emscripten](../emscripten)
3. For Emscripten, you can find an example from the `test-emscripte` job from `hermes/.circleci/config.yml`. Also see more details at [Building with Emscripten](./Emscripten.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing this is better because it works on both github.com and hermes website (with Docusaurus replacing the link to the correct url)?

Copy link
Contributor Author

@slorber slorber May 4, 2021

Choose a reason for hiding this comment

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

Docusaurus has a broken links checker now and this URL was reported as broken.

In general, I think it's a bad idea to link using relative URL paths, because it makes the link sensitive to your host provider adding an extra slash at the end of an URL or not (it is not something very std and lead to a ton of reported problems in all Jamstack frameworks) (our issue: facebook/docusaurus#3372)

Relative file paths are resolved to absolute URLs by Docusaurus at build time, so this is not sensitive to the host adding a trailing slash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(and yes, it also works on Github 👍 )

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh thanks for providing that context. I certainly fought with trailing slash appended by github pages before: https://twitter.com/Huxpro/status/798816417097224193

@@ -12,12 +12,12 @@
"clear": "docusaurus clear"
},
"dependencies": {
"@docusaurus/core": "^2.0.0-alpha.69",
"@docusaurus/preset-classic": "^2.0.0-alpha.69",
"@docusaurus/core": "2.0.0-alpha.75",
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be really strict. Is taking minor release w/ ^ bad?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not bad, it's just that after my tests using canary 2.0.0-alpha.hash somehow using ^ did not make yarn download alpha75 and the lockfile stayed on the canary version (probably because my version name is bad)

I can revert to ^ if you want but keep in mind we force users to have all core package versions aligned.
We'll add a "docusaurus upgrade" cli anyway so it does not matter much, up to you

Copy link
Contributor

@Huxpro Huxpro May 5, 2021

Choose a reason for hiding this comment

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

Thanks. No need to revert. We can always change that if we really need to.

@facebook-github-bot
Copy link
Contributor

@Huxpro has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@Huxpro merged this pull request in 44169f6.

@slorber
Copy link
Contributor Author

slorber commented May 5, 2021

👌 Thanks, happy to have one more site on webpack 5 🤗

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants