Closed alansouzati closed 6 years ago
I think this could be a problem with rollup and the way you are configuring acorn
as an external
Actually I think this is the culprit
https://github.com/RReverser/acorn-jsx/blob/ddcc01d707b2eb5f9ae4de36ba32e2276a18ba3c/index.js#L6
Actually I think this is a problem with conflicting acorn versions in the node_module tree. for some reason my node_modules/acorn
has version 5. and as this is being configured as an external, it is trying to read that, even though node_modules/buble/node_modules/acorn
has the version 6.
I have investigated on this issue a bit and here are my findings so far :
And I checked, no other version of acorn is loaded in this bundle.
in buble.es.js
, at the very top we can see :
import require$$0, { Parser } from 'acorn';
Which is the culprit as we now know acorn doesn't have a default export, and this if we look where it is used we get
// Line 263
var tt = require$$0.tokTypes;
var TokContext = require$$0.TokContext;
var tokContexts = require$$0.tokContexts;
var TokenType = require$$0.TokenType;
var isNewLine = require$$0.isNewLine;
var isIdentifierStart = require$$0.isIdentifierStart;
var isIdentifierChar = require$$0.isIdentifierChar;
// Line 741
require$$0.tokTypes._import.startsExpr = true;
// Line 746
if (this.type !== require$$0.tokTypes.parenL) {
// Line 771
if (this.type === require$$0.tokTypes._import && parenAfter.call(this)) {
// Line 783
if (this.type === require$$0.tokTypes._import) {
Which come respectively from
// acorn-jsx/index.js (For line 263)
const {tokTypes: tt, TokContext, tokContexts, TokenType, isNewLine, isIdentifierStart, isIdentifierChar} = require("acorn");
// acorn-dynamic-import/lib/index.js (For line 741 + 746`+ 771 + 783)
var _acorn = require('acorn');
_acorn.tokTypes._import.startsExpr = true;
if (this.type !== _acorn.tokTypes.parenL) {
if (this.type === _acorn.tokTypes._import && parenAfter.call(this)) {
if (this.type === _acorn.tokTypes._import) {
As you can see, in both cases, the require is done with require
and not import
. My conclusion is that the cause of this issue is rollup-plugin-commonjs
which doesn't convert those requires correctly.
My proposal to treat all node_modules as externals in the case of a browser build, Webpack can handle those imports fine.
I'll make a pull request for my proposed change
I just don't understand it. If this is a hard dependency why configure it as external? Why this is not a peerDependency then? I may be missing something here, but if you put it in "dependencies" it makes sense to me to bundle it together.
No, it's the other way round: If you don't configure it as external you shouldn't have it as dependency, since you are not actually require
ing the module during runtime.
is this a rollup concept? Sorry, I feel I'm definitely missing something, but, I'm using webpack + dependencies and I never had to configure external in webpack.
I let webpack bundle everything together and create separate chunks when my bundle size gets big.
I guess this is a rollup concept. With rollup, you can specify whether dependencies should be put in the bundle or referenced dynamically. If I don't specify any modules as external
, the bublé build is about 1MB and doesn't have any external dependencies. On the other hand, if I declare acorn
and magic-string
to be external, the bundled file has ~500KB and require
s acorn
and magic-string
on runtime, so they should be in dependencies
.
yeah, I think there is something weird with rollup and the way they are converting the require to import as @onigoetz said. This is a "blocker" issue for me, I had to remove the live code editor from https://v2.grommet.io. I will add it back once we figure this out.
Yes, external
is what you want for this, that's the nature of library dependencies (vs a product bundle where you're including all dependencies). Not specific to rollup, in rollup the option is called external
, in webpack it's called externals.
I hear you. In a project that I support I do declare externals for react and react dom
https://github.com/grommet/grommet/blob/NEXT/webpack.config.babel.js#L24
But they are peerDependencies in my package.json.
https://github.com/grommet/grommet/blob/NEXT/package.json#L41
I don't think this issue would be solved by using peerDependencies
.
That import in acorn-jsx
is wrong because acorn has no default exports.
I don't see a default import there.
See @onigoetz's comment on how that gets transpiled.
Ah, now I get it. Does this only happen with webpack?
I'm not sure, looking in to it. To be clear on the change from 0.19.4
release to 0.19.5
, it's this generated code:
0.19.4, acorn-jsx: ^4.1.1
import MagicString from 'magic-string';
import * as acorn from 'acorn';
0.19.5, acorn-jsx: ^5.0.0
import require$$0, { Parser } from 'acorn';
The default require$$$0
is what's causing the error with Webpack.
@alansouzati Externals and peerDependencies are two completely different concepts.
PeerDependencies are handled at npm level, it made sense back to npm 2, when the tree of dependency respected the definition of the packages, Those were a trick to allow multiple packages to leverage one dependency while only one needed to actually depend on it. With NPM 3, the need for peerDependencies is highly reduced as it is already able to flatten the dependency tree to install dependencies only once if possible. but having a dependency as a peerDepednency, devDependency or normal dependency makes no difference for the bundler you use (wether it's webpack, rollup, parcel or other.)
externals is a concept for bundlers, let me explain it this way, you provide a library with a react component that has prop-types, so you have a dependency on react
and prop-types
. If you bundle your application, you have two ways of compiling it
import React from "react";
and import PropTypes from "prop-types";
at the beginning.Option 1 is great when you are creating an application, it's the bundle that you're going to ship in the browser, it's logical to have everything inside. But in the case of a library, when importing it, if your application also requires React and PropTypes, it will have to re-import them in your bundle, and you will end up with code that is duplicated in your bundle without a way to remove it.
Option 2 on the other hand, leverages npm's dependencies, because it's able to resolve to the latest version of a dependency and your bundler is able to resolve your dependency and de-duplicate it for you The first case
Just tested with @onigoetz's PR #161 which fixes this. The resulting imports are:
import rewritePattern from 'regexpu-core';
import MagicString from 'magic-string';
import { Parser } from 'acorn';
import acornJsx from 'acorn-jsx';
import acornDynamicImport from 'acorn-dynamic-import';
Which works nicely with webpack.
I'm ambivalent on whether the other dependencies should be bundled or not, but definitely fully externalizing acorn/acorn-jsx solves the issue.
Does #162 fix it, too?
Fully externalizing dependencies guarantees that if any other library uses one of those, you don't end up importing them twice.
@adrianheine Actually, the best compilation for libraries ... is no compilation :) In general I just spit out the files transpiled but not bundled. it saves a lot of trouble : no bundler configuration, and the final bundler can work all its magic
@onigoetz I agree and I think I will do #161 in any case.
awesome :)
@alansouzati Externals and peerDependencies are two completely different concepts.
PeerDependencies are handled at npm level, it made sense back to npm 2, when the tree of dependency respected the definition of the packages, Those were a trick to allow multiple packages to leverage one dependency while only one needed to actually depend on it. With NPM 3, the need for peerDependencies is highly reduced as it is already able to flatten the dependency tree to install dependencies only once if possible. but having a dependency as a peerDepednency, devDependency or normal dependency makes no difference for the bundler you use (wether it's webpack, rollup, parcel or other.)
externals is a concept for bundlers, let me explain it this way, you provide a library with a react component that has prop-types, so you have a dependency on
react
andprop-types
. If you bundle your application, you have two ways of compiling it
- with everything inside: your library is going to ship with React and PropTypes inside.
- tell it to have everything as externals, which will create a bundle of your library with
import React from "react";
andimport PropTypes from "prop-types";
at the beginning.Option 1 is great when you are creating an application, it's the bundle that you're going to ship in the browser, it's logical to have everything inside. But in the case of a library, when importing it, if your application also requires React and PropTypes, it will have to re-import them in your bundle, and you will end up with code that is duplicated in your bundle without a way to remove it.
Option 2 on the other hand, leverages npm's dependencies, because it's able to resolve to the latest version of a dependency and your bundler is able to resolve your dependency and de-duplicate it for you The first case
Thanks for the great explanation. I agree they are unrelated.
@adrianheine I'm using Parcel
and I have the same error
Ok, so far my understanding is that
My current plan to fix this issue:
^acorn(-.*)?
for a quick fix releaseI'm assuming that my tests from #164 are enough to establish whether our build artifacts are correct or not, so if you have any further bundler setups that are broken, please post them.
Yeah a quick release with the bundled acorn-jsx is appreciated.
I just released v0.19.6 hopefully fixing this for now.
trying it now.
it works 👍 i think I was testing it with the old build or something.
also stumbled into this, please update when acorn-jsx is fixed, and acorn is out of the dist. 😃
I got around it by using import { transform } from 'buble/dist/buble-browser.cjs';
, but I don't want double acorn.
Please follow #165 for moving the acorn dependencies out of the bundle :)
I recently updated to the latest buble through react-live and my editor stopped working with the following error:
It seems that we are trying to use the default import from
acorn
that is gone.Later I get:
After inspecting, I realized that it is trying to read
tokTypes
fromacorn
.