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: use .css extension on react-loading-skeleton import #2362

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

davidjoy
Copy link
Contributor

@davidjoy davidjoy commented Jun 8, 2023

Folks are seeing errors like this in MFEs:

Module build failed (from ./node_modules/sass-loader/dist/cjs.js):
SassError: Can't find stylesheet to import.
  ╷
8 │ @import "~react-loading-skeleton/dist/skeleton";
  │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
  node_modules/@edx/paragon/scss/core/core.scss 8:9  @import
  src/index.scss 3:9                                 root stylesheet

react-loading-skeleton defines an exports field in its package.json which looks like this:

"exports": {
    ".": {
      "types": "./dist/index.d.ts",
      "require": "./dist/index.cjs",
      "import": "./dist/index.js"
    },
    "./dist/skeleton.css": "./dist/skeleton.css"
  },

The webpack documentation states that if an exports field is defined in package.json, it replaces the default module loading behavior and any other request for a module will result in a ModuleNotFound error.

Our call to load react-loading-skeleton in Paragon looks like this:

@import "~react-loading-skeleton/dist/skeleton";

That isn’t in the list of possible exports. This is though:

@import "~react-loading-skeleton/dist/skeleton.css";

This commit updates our import to the latter, which fixes downstream issues in MFEs that can’t figure out how to import react-loading-skeleton

Deploy Preview

Include a direct link to your changes in this PR's deploy preview here (e.g., a specific component page).

Merge Checklist

  • If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
  • Does your change adhere to the documented style conventions?
  • Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
  • Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
  • Were your changes tested in the example app?
  • Is there adequate test coverage for your changes?
  • Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

  • Verify your changes were released to NPM at the expected version.
  • If you'd like, share your contribution in #show-and-tell.
  • 🎉 🙌 Celebrate! Thanks for your contribution.

react-loading-skeleton defines an exports field in its package.json which looks like this:

"exports": {
    ".": {
      "types": "./dist/index.d.ts",
      "require": "./dist/index.cjs",
      "import": "./dist/index.js"
    },
    "./dist/skeleton.css": "./dist/skeleton.css"
  },

The webpack documentation states that if an exports field is defined in package.json, it replaces the default module loading behavior and any other request for a module will result in a ModuleNotFound error.

Our call to load react-loading-skeleton in Paragon looks like this:

@import "~react-loading-skeleton/dist/skeleton";

That isn’t in the list of possible exports.  This is though:

@import "~react-loading-skeleton/dist/skeleton.css";

This commit updates our import to the latter, which fixes downstream issues in MFEs that can’t figure out how to import react-loading-skeleton
@netlify
Copy link

netlify bot commented Jun 8, 2023

Deploy Preview for paragon-openedx ready!

Name Link
🔨 Latest commit 77fe415
🔍 Latest deploy log https://app.netlify.com/sites/paragon-openedx/deploys/64823d2a4b5a7300084e68e3
😎 Deploy Preview https://deploy-preview-2362--paragon-openedx.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@davidjoy
Copy link
Contributor Author

davidjoy commented Jun 8, 2023

I'm going to get this in so we can see if it officially fixes the issue in downstream MFEs once they upgrade to this version of Paragon. I was able to test it locally, but it'd be nice to have that validation. It may be that there's an upcoming fix in some webpack-related library that will make this unnecessary in the long run, but I wasn't able to find something. For now, it seems benign and solves problems.

@davidjoy davidjoy merged commit 979fc9d into master Jun 8, 2023
@davidjoy davidjoy deleted the davidjoy/react-loading-skeleton-import-fix branch June 8, 2023 20:57
@davidjoy
Copy link
Contributor Author

davidjoy commented Jun 8, 2023

OMG the list of checks was so long I didn't see that tests were still running when I merged. Ugh. Process fail.

@edx-semantic-release
Copy link
Contributor

🎉 This PR is included in version 20.43.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@monteri
Copy link
Contributor

monteri commented Jul 14, 2023

@davidjoy I would want to ask which Paragon version and which MFE is that where this issue is?

@davidjoy
Copy link
Contributor Author

Oof, that would have been good information to include.

So the issue is that Paragon didn't pin down a dependency on react-loading-skeleton, so it could affect any paragon version that had react-loading-skeleton at ~3.1.0 in its package JSON. Approximately 3 months ago react-loading-skeleton released version 3.3.0 which changed the way it exports its CSS, breaking any MFE that happened to upgrade to that version on an npm i.

react-loading-skeleton was introduced to Paragon in this PR: #1379, which was included in v20.4.0 of Paragon. So any MFE using that version through v20.43.2 of Paragon can break with this issue. At the time, we saw it manifesting in frontend-app-course-authoring on the master branch, but I recall some other folks occasionally running into it in other MFEs (though I can't find which ones).

@monteri
Copy link
Contributor

monteri commented Aug 16, 2023

@davidjoy There is an update for scss import to in this PR to solve the issue on the Paragon docs site.
The initial change @import "~react-loading-skeleton/dist/skeleton.css"; broke skeleton on the docs site so this PR is focused to find solution for both MFEs and Paragon docs site:

cmltaWt0 pushed a commit to raccoongang/paragon that referenced this pull request Aug 24, 2023
react-loading-skeleton defines an exports field in its package.json which looks like this:

"exports": {
    ".": {
      "types": "./dist/index.d.ts",
      "require": "./dist/index.cjs",
      "import": "./dist/index.js"
    },
    "./dist/skeleton.css": "./dist/skeleton.css"
  },

The webpack documentation states that if an exports field is defined in package.json, it replaces the default module loading behavior and any other request for a module will result in a ModuleNotFound error.

Our call to load react-loading-skeleton in Paragon looks like this:

@import "~react-loading-skeleton/dist/skeleton";

That isn’t in the list of possible exports.  This is though:

@import "~react-loading-skeleton/dist/skeleton.css";

This commit updates our import to the latter, which fixes downstream issues in MFEs that can’t figure out how to import react-loading-skeleton
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants