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

Failed to use yaml lib with webpack 5 #208

Closed
ghost opened this issue Oct 30, 2020 · 16 comments · Fixed by #224
Closed

Failed to use yaml lib with webpack 5 #208

ghost opened this issue Oct 30, 2020 · 16 comments · Fixed by #224
Labels
bug Something isn't working

Comments

@ghost
Copy link

ghost commented Oct 30, 2020

Got an error during webpack server command. And I can't figure out what I do wrong. If possible could you please help?

> webpack serve

ℹ 「wds」: Project is running at http://localhost:3333/
ℹ 「wds」: webpack output is served from undefined
ℹ 「wds」: Content not from webpack is served from /Users/igor/Projects/yaml-test/src
✖ 「wdm」: 1 asset
46 modules

ERROR in ./node_modules/yaml/browser/dist/index.js 1:0
Module parse failed: 'import' and 'export' may appear only with 'sourceType: module' (1:0)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
> import { d as defaultTagPrefix, _ as _createForOfIteratorHelper, a as _typeof, b as _createClass, c as _classCallCheck, e as _defineProperty, Y as YAMLSyntaxError, T as Type, f as YAMLWarning, g as YAMLSemanticError, h as _slicedToArray, i as YAMLError, j as _inherits, k as _createSuper } from './PlainValue-ff5147c6.js';
| import { parse as parse$1 } from './parse-cst.js';
| import { b as binaryOptions, a as boolOptions, i as intOptions, n as nullOptions, s as strOptions, N as Node, P as Pair, S as Scalar, c as stringifyString, A as Alias, Y as YAMLSeq, d as YAMLMap, M as Merge, C as Collection, r as resolveNode, e as isEmptyPath, t as toJSON, f as addComment } from './resolveSeq-04825f30.js';

webpack 5.3.2 compiled with 1 error in 4332 ms
ℹ 「wdm」: Failed to compile.

Webpack config:

const HtmlWebpackPlugin = require('html-webpack-plugin')

const path = require('path');

const srcDir = path.resolve(__dirname, 'src');

module.exports = {
  entry: `${srcDir}/index.js`,
  output: {
    path: path.resolve(__dirname, 'dist'),
    filename: '[name].[contenthash].js',
  },
  devtool: 'source-map',
  module: {
    rules: [
      {
        test: /\.js$/,
        exclude: /node_modules/,
        use: {
          loader: 'babel-loader',
          options: {
            presets: ['@babel/preset-env']
          }
        }
      }
    ]
  },
  plugins: [
    new HtmlWebpackPlugin({template: 'src/index.html'}),
  ],
  devServer: {
    port: 3333,
    contentBase: srcDir,
    stats: 'minimal',
    hot: true,
    inline: true,
  },
};

package.json

{
  "name": "yaml-test",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "dependencies": {
    "yaml": "^1.10.0"
  },
  "devDependencies": {
    "@babel/core": "^7.12.3",
    "@babel/preset-env": "^7.12.1",
    "@webpack-cli/serve": "^1.0.1",
    "babel-loader": "^8.1.0",
    "html-webpack-plugin": "^4.5.0",
    "webpack": "^5.3.2",
    "webpack-cli": "^4.1.0",
    "webpack-dev-server": "^3.11.0"
  },
  "scripts": {
    "start": "webpack serve"
  },
  "keywords": [],
  "author": "",
  "license": "ISC"
}

index.js

import YAML from 'yaml';

console.log(YAML.parse('1'));
@dusave
Copy link

dusave commented Oct 30, 2020

I have the same problem here.

@eemeli eemeli added the bug Something isn't working label Oct 31, 2020
@eemeli
Copy link
Owner

eemeli commented Oct 31, 2020

Huh, looks like Webpack 5 cares about the package.json "type" field, which it didn't before. Such will need to be added to the browser/dist/ directory of yaml, but in the meantime (or for yaml@1), adding this to your module.rules should make it work:

{ test: /\.js$/, type: 'javascript/auto' }

If you'd prefer to make the test really specific, use /node_modules\/yaml\/browser/.

Edit: Fixed specific regexp to grab all of yaml/browser.

@ghost
Copy link
Author

ghost commented Nov 2, 2020

@eemeli thanks. type: 'javascript/auto' works.

But more specific, as I understand, exclude does not work.

@eemeli
Copy link
Owner

eemeli commented Nov 2, 2020

@eemeli thanks. type: 'javascript/auto' works.

👍

But more specific, as I understand, exclude does not work.

I'm sorry, I don't think I quite understood what you meant here?

@ghost
Copy link
Author

ghost commented Nov 2, 2020

I thought you suggest to add /node_modules\/yaml\/browser\/dist\/.*/ to exclude (read inattentively 😄). But then I realize that I need to change rule test property. So in my work project I just add another rule to handle yamlspecifically.

{
  test: /node_modules\/yaml\/browser\/dist\/.*/,
  type: 'javascript/auto',
  use: {
    loader: 'babel-loader',
    options: {
      presets: [
        '@babel/preset-env',
      ],
    },
  },
}

Thanks

@dusave
Copy link

dusave commented Nov 10, 2020

@eemeli Confirmed this works! Thanks for the solution!

@eemeli eemeli closed this as completed in bc4929c Dec 29, 2020
@jedwards1211
Copy link

jedwards1211 commented Jan 15, 2021

@eemeli I think there's still a problem here...browser/index.js is requireing an ES module, which is not kosher anymore AFAIK (not in Node at least). Along these lines, I think webpack 5 expects required files to be script sourceType, which is why it says 'import' and 'export' may appear only with 'sourceType: module'.

package.json

  "browser": {
    "./index.js": "./browser/index.js",

./browser/index.js

module.exports = require('./dist').YAML

./browser/dist/index.js

import { d as defaultTagPrefix, _ as _createForOfIteratorHelper, a as _typeof, b as _createClass, c as _classCallCheck, e as _defineProperty, Y as YAMLSyntaxError, T as Type, f as YAMLWarning, g as YAMLSemanticError, h as _slicedToArray, i as YAMLError, j as _inherits, k as _createSuper } from './PlainValue-ff5147c6.js';
import { parse as parse$1 } from './parse-cst.js';

@eemeli
Copy link
Owner

eemeli commented Jan 15, 2021

@jedwards1211 Are you getting an error with Webpack 5 when using the type: 'javascript/auto' setting, or is it a matter of suspecting that some problem exists?

And you're right, you can't require() an ES module in Node.js, which is why that isn't done in the build of the library that Node.js gets. But it is fine with Webpack and Rollup -- unless I'm mistaken and you can provide some reproducible error case?

@jedwards1211
Copy link

jedwards1211 commented Jan 15, 2021

What I mean is I think if you make both files use the same source type, whether it's module or script, we wouldn't need type: 'javascript/auto' for webpack 5 to accept it, it would just work. By default webpack 5 no longer accepts it. Is there any reason not to use import/export in browser/index.js? (Or for that matter, point the package.json entries directly at browser/dist/...?)

@jedwards1211
Copy link

Okay I was experimenting with the installed package.json. Unfortunately even if I indicate ES modules like this:

  "exports": {
    ".": {
      "browser": {
        "import": "./browser/dist/index.js"
      }
    },
  },

Webpack 5 still complains 'import' and 'export' may appear only with 'sourceType: module', which seems like a bug.

To make matters worse Webpack 5 doesn't seem to interpret the condition-then-path style of nesting correctly:

  "exports": {
    "browser": {
      "import": {
        ".": "./browser/dist/index.js"
      }
    },
  },

This is a valid way of declaring conditional exports for Node at least according to https://nodejs.org/api/packages.html#packages_nested_conditions
but Webpack complains that "package path . is not exported" in this case.

@jedwards1211
Copy link

jedwards1211 commented Jan 16, 2021

Actually I was wrong, things have gotten too complicated 😭

Top level conditions are apparently only valid when there are no subpath exports:

  "exports": {
    "node": {
      "import": "./feature-node.mjs",
      "require": "./feature-node.cjs"
    },
    "default": "./feature.mjs",
  }

@jedwards1211
Copy link

jedwards1211 commented Jan 16, 2021

Okay, sorry for all the messages but I'm slowly figuring this out. The problem is "type": "commonjs" in your package.json causes all .js files to be parsed as CommonJS, including the ones that are supposed to be ES modules in browser/dist.

I had assumed that the "import" condition in conditional exports would override "type": "commonjs" and make Node and Webpack 5 treat the file as an ES Module, but bizarrely this is not the case. Apparently the sourceType is solely determined by the file extension and package "type" (in the case of .js).

It seems like the best thing would be to use .mjs for all ES modules in dist. Then Node and Webpack 5 would be more likely to interpret them correctly without special configuration.

Or you could output all CommonJS modules in browser, that wouldn't cause anyone trouble. I'm not sure dual packages are worth the trouble yet 😂

@eemeli
Copy link
Owner

eemeli commented Jan 16, 2021

As an experiment, could you apply locally the fix of bc4929c, which was the commit that closed this issue? As in, leave the root package.json untouched and add a file browser/dist/package.json with the following single line of contents?

{ "type": "module" }

@jedwards1211
Copy link

jedwards1211 commented Jan 16, 2021

Ah I didn't notice that commit. That would probably work for the root import, but it wouldn't work for some subpath imports because some of the files in browser/ (not browser/dist/) are ES modules. I don't understand why a mix of CommonJS and ES modules is being used inside browser/.

@eemeli
Copy link
Owner

eemeli commented Jan 16, 2021

Huh, I'd forgotten about browser/types.js and browser/util.js being ES. browser/index.js and browser/parse-cst.js were converted (back) to CommonJS due to #163, but that didn't affect the other two. In general, I much prefer keeping browser stuff as ES whenever possible, as bundlers generate clearly less scaffolding around such modules.

Will need to think of a solution for those files as well, and properly look at how Webpack 5 handles things.

@eemeli eemeli reopened this Jan 16, 2021
@jedwards1211
Copy link

jedwards1211 commented Jan 16, 2021

Okay I looked through #163 and the code, looks like you've changed the export structure in browser/dist some recently too.

I think this would work:

browser/package.json

{
  "type": "module"
}

browser/index.js

export * as default from './dist'
export * from './dist'

Then I believe all of the following ways of importing would work with bundlers (assuming they support all export from syntaxes?):

require('yaml').stringify(...)
import YAML from 'yaml'
YAML.stringify(...)
import * as YAML from 'yaml'
YAML.stringify(...)
import {stringify as stringifyYaml} from 'yaml'
stringifyYaml(...)

browser/parse-cst.js though

As far as browser/parse-cst.js, if we convert that to

export { parse as default } from './dist/parse-cst'

I forget whether bundlers will make require('yaml/parse-cst') return the parse function in that case?

This was referenced Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants