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

Production build does not handle node_modules containing es6 code #3865

Closed
1 of 4 tasks
TheTedAdams opened this issue Apr 30, 2019 · 9 comments
Closed
1 of 4 tasks

Production build does not handle node_modules containing es6 code #3865

TheTedAdams opened this issue Apr 30, 2019 · 9 comments
Labels
area:spfx Category: SharePoint Framework (not extensions related) area:tooling Category: Development tooling Needs: Attention 👋 Original poster responded to request for feedback, awaiting attention from Microsoft / community.

Comments

@TheTedAdams
Copy link

Category

  • Question
  • Typo
  • Bug
  • Additional article idea

Expected or Desired Behavior

Production build (--ship flag) is not broken by node_modules containing ES6 code.

Observed Behavior

Using SPFx 1.8, gulp serve works, but gulp build --ship does not, returning the following error:

webpart1.js from UglifyJs
Unexpected token: operator (>) [webpart1.js:54338,15]
webpart2.js from UglifyJs
Unexpected token: operator (>) [webpart2.js:53743,15]
webpart3.js from UglifyJs
Unexpected token: operator (>) [webpart3.js:51210,15]

Based on the absurd line numbers and the .js extension this seems to be happening after typescript and after the bundle has been concatenated together... Given that my tsconfig targets ES5 my belief is a node_module shipping es6 code (which is starting to happen more frequently as library maintainers abandon IE support) is being put in the bundle, which is then being run through uglifyjs-webpack-plugin. uglifyjs-webpack-plugin, being tied to uglify-js, does not support es6 syntax.

Resolution suggestions

I see two possible resolutions.

  1. Switch to TerserWebpackPlugin (or terser-webpack-plugin-legacy if you insist on sticking with webpack@3). Webpack@4 has switched to using terser by default when mode is 'production' due to uglify-js deciding not to support ES6 syntax, so when we migrate to Webpack 4 we'll most likely be using terser anyway.
  2. [BETTER] For greater safety, node_modules should really be run through babel to allow us to safely target ES5. create-react-app does this by adding a loader just for node_modules to their webpack config. As more package authors decide to drop IE support and start shipping ES6 this will be necessary for SPFx developers to be able to safely use node modules.

Steps to Reproduce

Use a node_module that has begun shipping their distributed .js in ES6.
Attempt to gulp build --ship
Observe UglifyJs errors.

This is likely the same bug as #1422

@msft-github-bot
Copy link
Collaborator

Thank you for reporting this issue. We will be triaging your incoming issue as soon as possible.

@msft-github-bot msft-github-bot added the Needs: Triage 🔍 Awaiting categorization and initial review. label Apr 30, 2019
@TheTedAdams
Copy link
Author

For easy repro my build was being broken by query-string which started shipping ES6 in v6

@semopz
Copy link

semopz commented May 9, 2019

@TheTedAdams, Any tips on how to get around this issue when using query-string? I'm working on a SPFx 1.8 project.

My workaround is to install version 5.1.1. You still get an error "The build failed because a task wrote output to stderr." but at least the package you get in the end seems to work.

If you have a better workaround, please let me know.

@TheTedAdams
Copy link
Author

@semopz yes for now I did the same thing, npm i query-string@5 until SPFx build adds graceful handling for this. I know that in my main application I use several packages that distribute ES6 though, so this is something that is going to need to be handled in the build process.

I assume if you wanted to handle it yourself you could edit your gulpfile.js to add a loader to run node_modules through babel. I'm not sure if I've really seen it documented around but in another github issue someone mentioned you could modify the webpack config like so:

'use strict';

const gulp = require('gulp');
const build = require('@microsoft/sp-build-web');
build.addSuppression(`Warning - [sass] The local CSS class 'ms-Grid' is not camelCase and will not be type-safe.`);

build.configureWebpack.mergeConfig({
  additionalConfiguration: (generatedConfiguration) => {
    generatedConfiguration.externals.splice(generatedConfiguration.externals.indexOf('react'), 1);
    generatedConfiguration.externals.splice(generatedConfiguration.externals.indexOf('react-dom'), 1);
    return generatedConfiguration;
  }
});

build.initialize(gulp);

Obviously this was to remove external react references so that I could ship w/ react@16... but it highlights the way you can modify the webpack configuration to suit your needs, and you could probably use this to add a babel loader. I just haven't tried yet.

@lucabandMSFT lucabandMSFT added area:community and removed Needs: Triage 🔍 Awaiting categorization and initial review. labels May 9, 2019
@semopz
Copy link

semopz commented May 15, 2019

Cool thanks. I'm not sure if I'll be trying the babel approach as query-string is the only thing that's impacting my project. npm i query-string@5 installed version 5.0... for me which had a bug (I forget what) so I had to manually bump it to the latest v5 in package.json

I see that a 'community' label has been tagged on this issue. I have no idea what the labels mean in this repo. Does it mean that this issue is not considered a bug or it won't be fixed?

@andrewconnell
Copy link
Collaborator

Not sure this is still an issue... I can't repro with 1.9.1...

@andrewconnell andrewconnell added area:spfx Category: SharePoint Framework (not extensions related) area:tooling Category: Development tooling Needs: Author Feedback Awaiting response from the original poster of the issue. Marked as stale if no activity for 7 days. and removed area:community labels Nov 1, 2019
@TheTedAdams
Copy link
Author

The move to WebPack v4 means that things seem to now be uglified using the Terser plugin, which as I mentioned does not die on ES6 code.

The current SPFx build does NOT do anything to make node_modules safe (no babel loader or anything like that) but it no longer breaks. If you are fine shipping code to SharePoint that doesn't have to run in IE... you'll be fine and you work at a cool company.

Closing.

@msft-github-bot msft-github-bot added Needs: Attention 👋 Original poster responded to request for feedback, awaiting attention from Microsoft / community. and removed Needs: Author Feedback Awaiting response from the original poster of the issue. Marked as stale if no activity for 7 days. labels Nov 1, 2019
@TheTedAdams
Copy link
Author

I also just wanted to add here in case anybody finds this via search, I was able to add a babel-loader for node_modules if you want to use es6 node_modules that ship in ES6, like query-string v6+

build.configureWebpack.mergeConfig({
  additionalConfiguration: generatedConfiguration => {
    // Add a babel loader to transform node_modules to be IE safe
    process.env.BABEL_ENV = 'production';
    generatedConfiguration.module.rules.push({
      test: /\.(js)$/,
      include: /node_modules/,
      exclude: /@babel(?:\/|\\{1,2})runtime|core-js|react|react-dom/,
      loader: require.resolve('babel-loader'),
      options: {
        babelrc: false,
        configFile: false,
        compact: false,
        presets: [
          [
            '@babel/preset-env',
            {
              useBuiltIns: false,
            },
          ],
        ],
        cacheDirectory: true,
        cacheCompression: true,
        sourceMaps: false,
      },
    });

    return generatedConfiguration;
  },
});

@msft-github-bot
Copy link
Collaborator

Issues that have been closed & had no follow-up activity for at least 7 days are automatically locked. Please refer to our wiki for more details, including how to remediate this action if you feel this was done prematurely or in error: Issue List: Our approach to locked issues

@SharePoint SharePoint locked as resolved and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area:spfx Category: SharePoint Framework (not extensions related) area:tooling Category: Development tooling Needs: Attention 👋 Original poster responded to request for feedback, awaiting attention from Microsoft / community.
Projects
None yet
Development

No branches or pull requests

5 participants