Skip to content
This repository has been archived by the owner on Jan 26, 2019. It is now read-only.

"Unexpected token" error if importing jsx #146

Closed
pmunin opened this issue Aug 22, 2017 · 23 comments
Closed

"Unexpected token" error if importing jsx #146

pmunin opened this issue Aug 22, 2017 · 23 comments

Comments

@pmunin
Copy link

pmunin commented Aug 22, 2017

Is this a bug report?

Yes

Can you also reproduce the problem with npm 4.x?

Yes

Which terms did you search for in User Guide?

I search for different combinations of keywords describing using react-scripts-ts for app with both tsx and jsx modules

Environment

  1. npm ls react-scripts-ts (if you haven’t ejected): react-scripts-ts@2.6.0
  2. node -v: v8.2.1
  3. npm -v: 5.3.0
  4. yarn --version (if you use Yarn): 0.27.5
    <<<<<<< HEAD
  5. npm ls react-scripts (if you haven’t ejected):
    =======
  6. npm ls react-scripts-ts (if you haven’t ejected):

Fix Code Review

Then, specify:

  1. Operating system: Win7 SP1 x64
  2. Browser and version (if relevant): Chrome 60

Steps to Reproduce

I have application with mixed js(x) and ts(x) modules, which I need to coexist and compile properly, however when I rename any of tsx modules to js or jsx, I get the build error:

  1. create-react-app my-app
  2. rename src/App.tsx => src/App.jsx
  3. npm run build

Expected Behavior

Successfully compiled build/dist

Actual Behavior

Actual results:

./src/App.jsx
Module parse failed: c:\temp\test-react-typescript\src\App.jsx Unexpected token (9:6)
You may need an appropriate loader to handle this file type.
|   render() {
|     return (
|       <div className="App">
|         <div className="App-header">
|           <img src={logo} className="App-logo" alt="logo" />

Reproducible Demo

Please just follow steps, it's very easy to reproduce.

@pmunin
Copy link
Author

pmunin commented Aug 22, 2017

I looked at sources and I see that for some reason, script-ts' webpack.config.ts have no babel loader mentioned at all, which looks like a bug to me. It makes sense for react-scripts-ts to be a superset of react-scripts and include everything what react-scripts includes plus ts-loader.

Workaround I used - is just to use npm run eject and do it manually - it would be great if it can become out of the box

@joshhornby
Copy link

I agree with @pmunin I'd love to use this package replace CRA and help migrate to TypeScript, seems a bit extreme to have to eject just to be able to do this.

@wmonk
Copy link
Owner

wmonk commented Aug 22, 2017

Hey, thanks for this report. Currently this is entirely intentional, not a bug. react-scripts-ts is not acting as a superset of react-script at the moment, it is a complete replacement for typescript applications.

The original intention of the project was to be a replacement, which meant that it fell into the category of starting a new project with typescript support, instead of converting existing CRA generated projects - which I think is the same for the original CRA project too (new projects, instead of converting old ones).

There is a PR #74 that would go further to enabling this behaviour. My major concern with adding babel and JS support is that it would open up a whole range of potential new issues and mean support for more tooling, which isn't easy to maintain.

@pmunin
Copy link
Author

pmunin commented Aug 22, 2017

As an idea for a cheap and flexible fix - add ability to customize/override of webpack.config.js in the project folder, like it works for tsconfig.json. Ideally - expect a function accepting as a parameter preinitialized (default) configuration object, where function returns overridden config, so developer could do something like:

//webpack.config.js (in project folder)
module.exports = function(defaultConfig, env)
{
   defaultConfig.module.rules.push({ 
      test: \/.jsx\/,
      loader:"babel-loader"
   });
   return defaultConfig;
}

This override will work both for devserver and prod build.

PS: I believe it's very low chances to meet a real-world app that's purely typescripts with no js included at all. And if es6 js included, then babel and build customization is needed.

@wmonk
Copy link
Owner

wmonk commented Aug 22, 2017

I'm afraid that is not going to land in this project, adding something like this is deviating far from the original react-scripts project, which is something I definitely don't want to do. I would like to open this up for discussion to see what other users are thinking. If this is a feature many want, then we will support it as the existing react-script package does. My gut feeling is that this will open up a whole host of issues. I also don't believe that it is completely unreasonable to eject and add this behaviour. Once you have completely completed the process of moving to TS then you can utilise this project again.

PS: I believe it's very low chances to meet a real-world app that's purely typescripts with no js included at all. And if es6 js included, then babel and build customization is needed.

I'm not sure this is necessarily true. Possibly for converting existing JS projects to TS. Considering the amount of downloads this package has had, this is the first (I think) issue of this nature.

@pmunin
Copy link
Author

pmunin commented Aug 22, 2017

I'm not sure this is necessarily true. Possibly for converting existing JS projects to TS. Considering the amount of downloads this package has had, this is the first (I think) issue of this nature.

Hmm, here is the scenario that I believe should be common: User's typescript imports some 3rd party js-module (e.g. that uses es6 or some latest js features). In this case, webpack trying to bundle it into the same js output and eventually app will fail in runtime, because latest js features of 3rd party were not babel-ed out? Am I missing something?

@wmonk
Copy link
Owner

wmonk commented Aug 22, 2017

That shouldn't happen anyway, as generally we don't run babel (or typescript) across 3rd party dependencies. If you definitely want that behaviour you will have to eject.

facebook/create-react-app#1125 this thread from create-react-app also gives the same advice.

@pmunin
Copy link
Author

pmunin commented Aug 22, 2017

I see your point, however many learning materials about webpack that I saw mentioned bunding vendor libs into dist as something common.

Another problem I'm dealing right now, that makes my point: missing/inconsistent typing. I'm dealing with material-ui package now, where recommended version to use is 1.0.0-beta, when latest Defined typings are v 0.18.0, which causes typescript compilation errors. So for those files where I'm using components with problematic types I could use JSX, but for the rest - I would use typescript.

@joshhornby
Copy link

joshhornby commented Aug 22, 2017

I am currently using react-app-rewired and doing the following:

// config-overrides.js
var paths = require('react-scripts-ts/config/paths');

module.exports = function override(config) {
    config.module.rules.push({
        test: /\.(js|jsx)$/,
        include: paths.appSrc,
        loader: require.resolve('babel-loader'),
        options: {
            babelrc: false,
            presets: [require.resolve('babel-preset-react-app')],
            cacheDirectory: true,
        },
    });

    return config
}

and in package.json "start": "react-app-rewired start --scripts-version react-scripts-ts",

This allows me to migrate my app file by file across to typescript.

@pmunin
Copy link
Author

pmunin commented Aug 22, 2017

Oh great, this is exactly what I suggested as a cheap fix above. Didn't know there is already existing solution. Thanks!

@wmonk
Copy link
Owner

wmonk commented Aug 23, 2017

Glad this is solved!

@wmonk wmonk closed this as completed Aug 23, 2017
@n8sabes
Copy link

n8sabes commented Aug 30, 2017

Thanks @joshhornby @pmunin, this workaround solves the problem.

Just needs to do some tree shaking uglification as the build output is massive when including JSX as-is.

It would be nice to incorporate this as a standard behavior of the react-scrips-ts as jsx is often a requirement at some point within larger projects.

@n8sabes
Copy link

n8sabes commented Aug 30, 2017

@joshhornby do you have any insight into how to reduce the (massive) size of the babel output when including jsx? I assume tree-shaking is occurring with tsx, but not on jsx.

@pmunin
Copy link
Author

pmunin commented Aug 30, 2017

@wmonk, I agree with @n8sabes after working more with it. Every time I import 3rd party module it gets embedded in main.js + source map bundle, which makes it huge and long to build. You mentioned that 3rd party modules are not supposed to included in the main.js generated, but what is the solution create-react-app provides to use import * as 3drPartyLib from '3rdPartyLib' without touching public folder (i.e. without including it in public\index.html separate script tag)?

@n8sabes
Copy link

n8sabes commented Aug 30, 2017

@pmunin, would you mind reopening this issue to run this to ground with a complete solution -- and hopefully tiny output?

@wmonk
Copy link
Owner

wmonk commented Aug 30, 2017

I'm not quite sure what your issue is now? I guess the reason it's taking so long is because you're not running babel across every single JS file.

Which modules are you using that are distributing raw JSX?

@pmunin
Copy link
Author

pmunin commented Aug 30, 2017

Here is example: component react-handsontable. Importing it - adds additional 2 minutes to every single build (even if I changed a single char in my code).

Another example component: react-icons if i import like import * as fa from 'react-icons/lib/fa - same story adds another additional 2 minutes to build time and + 5Mb to main.js.

What is the best way to fix it?

ps: react-scripts-ts does not use babel at all, isnt' it?

@n8sabes
Copy link

n8sabes commented Aug 30, 2017

@wmonk check out this example. I've included both a TSX and JSX Widget.

  1. Build AS-IS, which includes the JSX Widget and produces a very large output. The main[id].js is 1.3MB.

  2. Remove the JSX Widget import and implementation (2 lines) from the App.tsx and build again. This time it will produce a reasonable output. The main[id].js is 405KB.

@wmonk
Copy link
Owner

wmonk commented Aug 30, 2017

@pmunin Both of these libs distribute their code precompiled, what do you need babel for here?

@n8sabes I really don't know what is causing that bloat. But this is exactly the reason why adding babel compilation is something I didn't want to do.

@n8sabes
Copy link

n8sabes commented Aug 30, 2017

@wmonk -- indeed.

End of the day, it's a matter of how we support .jsx as part of a CRA TypeScript project. While I always prefer TypeScript, there are times in a large project (like now) that some JSX is required rather than full code port / rewrite.

I am not familiar enough with the react-script override mechanism to correct this.

@pmunin
Copy link
Author

pmunin commented Aug 30, 2017

@wmonk, I wasn't talking about babel - it was just a PS, because you mentioned:

the reason it's taking so long is because you're not running babel across every single JS file.

@pmunin Both of these libs distribute their code precompiled, what do you need babel for here

It's still not clear for me - how to reduce the build time if I import those libs?

@n8sabes
Copy link

n8sabes commented Aug 30, 2017

While I would prefer a more elegant solution via a CRA script, here is a workaround to generate an optimized build. It does not reduce build time.

yarn add -D webpack-parallel-uglify-plugin

  // config-overrides.js
  const ParallelUglifyPlugin = require('webpack-parallel-uglify-plugin');

  config.module.rules.push({
    test: /\.(js|jsx)$/,
    include: paths.appSrc,
    loader: require.resolve('babel-loader'),
    options: {
      babelrc: true,
      presets: [require.resolve('babel-preset-react-app')],
      cacheDirectory: true,
    },
  });

  config.plugins.push(new ParallelUglifyPlugin({
    sourceMap: true,
    uglifyES: {}
  }));

Be sure to have a valid .babelrc

See this boilerplate for a working example.

@icopp
Copy link

icopp commented Sep 18, 2017

@wmonk The basic problem is that Babel does tree shaking, and Typescript's direct compilation to JS doesn't. So, if you only use ts-loader or similar, you're pulling in the entire contents of every module you import. This means that react-scripts-ts produces bundles larger (potentially much, much larger) than react-scripts does.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants