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

Create React App doesn't work out of the box with Shoelace #1049

Closed
ajmchambers opened this issue Dec 7, 2022 · 21 comments · Fixed by #1050
Closed

Create React App doesn't work out of the box with Shoelace #1049

ajmchambers opened this issue Dec 7, 2022 · 21 comments · Fixed by #1050
Assignees
Labels
bug Things that aren't working right in the library. help wanted Ready for a contributor to tackle.

Comments

@ajmchambers
Copy link
Contributor

ajmchambers commented Dec 7, 2022

UPDATE: The original issue mentioned below has been resolved, but a new issue was uncovered regarding Create React App + Shoelace. See below for more details.

Jump to the latest issue 👇


View original issue

Describe the bug

After updating to 2.0.0-beta.86, getting import error (probably due to recent package.json exports field changes, using vite 3.2.5).

Importing like this:

import { SlButton } from "@shoelace-style/shoelace/dist/react";

Produces this error:

Internal server error: Missing "./dist/react" export in "@shoelace-style/shoelace" package

This can be fixed by changing the import to this:

import { SlButton } from "@shoelace-style/shoelace/dist/react/index.js";

Possible solutions

  1. Update documentation to use the '@shoelace-style/shoelace/dist/react/index.js' import
  2. Might be able to maintain current behaviour by modifying the package.json exports field to add this:
{
    "./dist/react": "./dist/react/index.js",
}
  1. Could output a ./dist/react.js file, add that to the exports and import using @shoelace-style/shoelace/dist/react.js
@ajmchambers ajmchambers added the bug Things that aren't working right in the library. label Dec 7, 2022
@ajmchambers

This comment was marked as outdated.

@claviska

This comment was marked as outdated.

@BasyaLipman

This comment was marked as outdated.

@alexey13

This comment was marked as outdated.

@claviska

This comment was marked as outdated.

@ajmchambers
Copy link
Contributor Author

ajmchambers commented Dec 8, 2022

The CRA issue described with errors about needing: '@babel/plugin-transform-classes' seems unrelated to the package.json exports changes. I installed @shoelace-style/shoelace 2.0.0-beta.83 (version before the exports changes) in a newly created CRA project and get the same error.


Now haven't touched create-react-app in a while, but was able to get the error to go away in CRA in two ways (not sure I'd recommend either approach though):

1. Customising babel with 'craco'

Was able to get it working using https://craco.js.org/ to customise the babel config, roughly this:

npm install --save-dev @craco/craco @babel/plugin-proposal-class-properties @babel/plugin-transform-classes

adjust package.json scripts to use craco:

{
  "scripts": {
    "start": "craco start",
    "build": "craco build",
    "test": "craco test"
}

Create a craco.config.js file with this:

module.exports = {
  babel: {
    plugins: [
      ["@babel/plugin-proposal-class-properties", { loose: true }],
      "@babel/plugin-transform-classes",
    ],
  },
};

2. Changing "browserslist" in package.json

After reading this: facebook/create-react-app#11339 (comment)

changing the browserslist in package.json to just "last 1 chrome version", it seems to work in development at least:

{
  "browserslist": {
    "production": [
      ">0.2%",
      "not dead",
      "not op_mini all"
    ],
    "development": [
      "last 1 chrome version"
    ]
  }

So it seems CRA configures it's babel settings depending on the specified browser support, and the out-of-the-box settings are configuring it in a way that doesn't like shoelace.


Haven't done much testing beyond this... but yea it would probably be good if shoelace worked out of the box in CRA as it's probably still pretty popular (despite in my opinion, better alternatives like vite's react preset etc). Not sure what needs changed though to make it work.

@ajmchambers
Copy link
Contributor Author

@claviska

This comment was marked as outdated.

@claviska claviska reopened this Dec 8, 2022
BasyaLipman pushed a commit to BasyaLipman/shoelace that referenced this issue Dec 8, 2022
@alexey13

This comment was marked as outdated.

@BasyaLipman
Copy link

BasyaLipman commented Dec 8, 2022

Yes, I can also confirm that changing "browserslist" in package.json of the CRA worked. Thank you!

@ajmchambers
Copy link
Contributor Author

Adding the "useDefineForClassFields": false, to tsconfig.json did not fix the issue. Maybe worth adding anyway for the typescript output.


Did some digging, but no solutions.

After running build we have this output (picked this file as it was the first errored from CRA).

Example of /dist/internal/formdata-event-polyfill.js:

"use strict";
class FormDataEventPolyfill extends Event {
  constructor(formData) {
    super("formdata");
    this.formData = formData;
  }
}
class FormDataPolyfill extends FormData {
  constructor(form) {
    if (form) {
      super(form);
      this.form = form;
      form.dispatchEvent(new FormDataEventPolyfill(this));
    } else {
      super();
    }
  }
  append(name, value) {
    if (!this.form) {
      return super.append(name, value);
    }
    let input = this.form.elements[name];
    if (!input) {
      input = document.createElement("input");
      input.type = "hidden";
      input.name = name;
      this.form.appendChild(input);
    }
    if (this.has(name)) {
      const entries = this.getAll(name);
      const index = entries.indexOf(input.value);
      if (index !== -1) {
        entries.splice(index, 1);
      }
      entries.push(value);
      this.set(name, entries);
    } else {
      super.append(name, value);
    }
    input.value = value;
  }
}

And there is also this chunk (which is created by esbuild I guess) /dist/internal/chunks/chunk.JHBEU2I5.js:

// src/internal/formdata-event-polyfill.ts
var FormDataEventPolyfill = class extends Event {
  constructor(formData) {
    super("formdata");
    this.formData = formData;
  }
};
var FormDataPolyfill = class extends FormData {
  constructor(form) {
    var __super = (...args) => {
      super(...args);
    };
    if (form) {
      __super(form);
      this.form = form;
      form.dispatchEvent(new FormDataEventPolyfill(this));
    } else {
      __super();
    }
  }
  append(name, value) {
    if (!this.form) {
      return super.append(name, value);
    }
    let input = this.form.elements[name];
    if (!input) {
      input = document.createElement("input");
      input.type = "hidden";
      input.name = name;
      this.form.appendChild(input);
    }
    if (this.has(name)) {
      const entries = this.getAll(name);
      const index = entries.indexOf(input.value);
      if (index !== -1) {
        entries.splice(index, 1);
      }
      entries.push(value);
      this.set(name, entries);
    } else {
      super.append(name, value);
    }
    input.value = value;
  }
};

The error in create-react-app refers to that chunk:

ERROR in ../shoelace/dist/chunks/chunk.JHBEU2I5.js

Module build failed (from ./node_modules/babel-loader/lib/index.js):
SyntaxError: C:\Development\shoelace\dist\chunks\chunk.JHBEU2I5.js: When using '@babel/plugin-transform-parameters', it's not possible to compile `super()` in an arrow function with default or rest parameters without compiling classes.
Please add '@babel/plugin-transform-classes' to your Babel configuration.
  13 |   constructor(form) {
  14 |     var __super = (...args) => {
> 15 |       super(...args);
     |       ^^^^^^^^^^^^^^
  16 |     };
  17 |     if (form) {
  18 |       __super(form);

That error message was added in babel because of this: babel/babel#15148


So kinda back where we started, create-react-app combined with the defined browserslist is configuring babel in such a way where it runs into the above issue with the above code output by esbuild. Or something like that... ¯\_(ツ)_/¯

@claviska
Copy link
Member

claviska commented Dec 8, 2022

Pinging @samuelstroschein for any insights. I'd hate to have to roll back #1007 but we need to get this sorted for React users. 😕

@ajmchambers
Copy link
Contributor Author

Thats the thing though, the same issue occurs in @shoelace-style/shoelace@2.0.0-beta.83 the version before that change, so it may have been broken for create-react-app (react-scripts: 5.0.1) before that change was even made, it's only now been discovered

@claviska
Copy link
Member

claviska commented Dec 8, 2022

Is this a React thing then? What version? I've been testing the last few versions of Shoelace in a CRA I created some time ago using React 17.0.2 and never encountered an issue.

@ajmchambers
Copy link
Contributor Author

ajmchambers commented Dec 8, 2022

I attempted to recreate the issue in code-sandbox but couldn't reproduce it there, had to do it on my machine directly:

npx create-react-app cra-app
cd cra-app
npm install --save @shoelace-style/shoelace@2.0.0-beta.83

edit /src/App.js to be this:

import { SlButton } from '@shoelace-style/shoelace/dist/react'

function App() {
  return (
    <div className="App">
      <SlButton>Test</SlButton>
    </div>
  );
}

export default App;
npm start

Dependencies look like this:

  "dependencies": {
    "@shoelace-style/shoelace": "^2.0.0-beta.83",
    "@testing-library/jest-dom": "^5.16.5",
    "@testing-library/react": "^13.4.0",
    "@testing-library/user-event": "^13.5.0",
    "react": "^18.2.0",
    "react-dom": "^18.2.0",
    "react-scripts": "5.0.1",
    "web-vitals": "^2.1.4"
  },

Node: v18.11.0
OS: Windows 11

Edit: https://stackblitz.com/edit/node-qdvgun?file=package.json,src%2FApp.js

@samuelstroschein

This comment was marked as outdated.

@claviska

This comment was marked as outdated.

@claviska claviska added the help wanted Ready for a contributor to tackle. label Dec 8, 2022
@claviska
Copy link
Member

claviska commented Dec 9, 2022

changing the browserslist in package.json to just "last 1 chrome version", it seems to work in development at least:

I just setup a fresh create-react-app project with the latest version, 5.0.1, and here's the default, unmodified browserslist in package.json:

  "browserslist": {
    "production": [
      ">0.2%",
      "not dead",
      "not op_mini all"
    ],
    "development": [
      "last 1 chrome version",
      "last 1 firefox version",
      "last 1 safari version"
    ]
  }

Unfortunately, this error still occurs when importing from Shoelace:

Module build failed (from ./node_modules/babel-loader/lib/index.js):
SyntaxError: /Users/claviska/Projects/shoelace/dist/chunks/chunk.JHBEU2I5.js: When using '@babel/plugin-transform-parameters', it's not possible to compile `super()` in an arrow function with default or rest parameters without compiling classes.
Please add '@babel/plugin-transform-classes' to your Babel configuration.

The quick fix for users is adding @babel/plugin-transform-classes but I'd like to see if we can solve this without the user having to do any of this.

@claviska claviska changed the title @shoelace-style/shoelace/dist/react vite import error after package.json exports changes Create React App doesn't work out of the box with Shoelace Dec 9, 2022
@claviska
Copy link
Member

claviska commented Dec 9, 2022

Adding the "useDefineForClassFields": false, to tsconfig.json did not fix the issue. Maybe worth adding anyway for the typescript output.

Added in 6284ed0 per Lit's recommendation so this doesn't bite us if and when we switch to es2022 or higher.

Of course, the issue above still remains.

@claviska claviska added this to the 2.0 (stable) milestone Dec 9, 2022
@Panda-On-Tree
Copy link

Hey , Where do i have to add @babel/plugin-transform-classes to get it fixed?

@claviska
Copy link
Member

Thanks, everyone. I just tried from scratch and it seems to be working fine with 2.0.0-beta.87. I also decided to make a video showing how to setup a CRA with Shoelace.

https://www.youtube.com/watch?v=Ku2mJCJgvLs

I'm going to close this since it seems to be resolved now. If anyone is still having an issue with this, please make sure you're using 2.0.0-beta.87 and report back if the problem persists for you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Things that aren't working right in the library. help wanted Ready for a contributor to tackle.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants