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(core): reapply resolve.symlinks: false #6604

Closed
wants to merge 1 commit into from
Closed

Conversation

Josh-Cena
Copy link
Collaborator

Motivation

Fix #6257. Since the Webpack bug is fixed, we can now reapply this config.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Removed the real path resolution in our dogfooding

Related PRs

#5126

@Josh-Cena Josh-Cena added the pr: bug fix This PR fixes a bug in a past release. label Feb 4, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 4, 2022
@netlify
Copy link

netlify bot commented Feb 4, 2022

✔️ [V2]

🔨 Explore the source changes: 9988fb4

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61fd039832d27700073cecf5

😎 Browse the preview: https://deploy-preview-6604--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Feb 4, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 92
🟢 Accessibility 100
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 92

Lighthouse ran on https://deploy-preview-6604--docusaurus-2.netlify.app/

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Feb 4, 2022

@slorber This config doesn't work with PNPM. Should we just make it configurable instead? Or do you think using a plugin to configure webpack is sufficient?

@github-actions
Copy link

github-actions bot commented Feb 4, 2022

Size Change: +194 B (0%)

Total Size: 772 kB

Filename Size Change
website/build/assets/js/main.********.js 583 kB +194 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 47.2 kB
website/build/assets/css/styles.********.css 105 kB
website/build/index.html 36.7 kB

compressed-size-action

@slorber
Copy link
Collaborator

slorber commented Feb 4, 2022

😅

The webpack config is good enough but having this by default can be useful to avoid users form thinking about it. I think it's @felipecrs that mentioned this recently somewhere.

I don't know what might be the problem with pnpm unfortunately

@Josh-Cena
Copy link
Collaborator Author

PNPM uses hard links for all packages because they keep a global package storage. Webpack resolve.symlinks: false is known to fail with symlinked packages (similarly with npm link).

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Feb 4, 2022

Also, the conversation you've been looking for is in that linked PR in the description. You seem to like injecting configuration with a plugin? Maybe we can promote this to a webpack.resolveSymlinks config option?

@slorber
Copy link
Collaborator

slorber commented Feb 4, 2022

PNPM uses hard links for all packages because they keep a global package storage.

Does it mean resolve.symlinks: false will never work with pnpm in any case?

Ping @zkochan, can you help us figure this out please?


You seem to like injecting configuration with a plugin? Maybe we can promote this to a webpack.resolveSymlinks config option?

If we add a shortcut, I'd rather add webpack.configure first ?

Not as convenient but generic and reusable in other cases.

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Feb 4, 2022

Docs on symlinked packages: https://webpack.js.org/configuration/resolve/#resolvesymlinks

Note that this may cause module resolution to fail when using tools that symlink packages (like npm link).

This includes PNPM which symlinks all the time

If we add a shortcut, I'd rather add webpack.configure first ?

Cool as well

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Mar 25, 2022

Going to close this. I think the plugin lifecycle is sufficient and with inline plugin modules, the bar to creating a plugin is extremely low:

// docusaurus.config.js
module.exports = {
  // ...
  plugins: [
    () => ({
      configureWebpack() {
        return { resolve: { symlinks: false } };
      },
    }),
  ],
};

@Josh-Cena Josh-Cena closed this Mar 25, 2022
@Josh-Cena Josh-Cena deleted the jc/resolve-symlink branch March 25, 2022 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Symlink docs folder not working
3 participants