DeMoorJasper / parcel-plugin-svelte

A parcel plugin that enables svelte support
MIT License
231 stars 31 forks source link

Class constructor SvelteComponent cannot be invoked without 'new' #44

Open leny opened 5 years ago

leny commented 5 years ago

šŸ› Bug Report

Hi, got some strange error with a basic "Hello world" app.

Class constructor SvelteComponent cannot be invoked without 'new', pointing my app.js

šŸŽ› Configuration (.svelterc, package.json, cli command)

Nothing special.

šŸ’» Code Sample

Everything's in the repo, but the code is quite simple :

import App from "./components/app.svelte";

const root = new App({
    target: document.querySelector("#root"),
    props: {},
});

export default root;

and the app.svelte is:

<script>
    let name = 'world';
</script>

<h1>Hello {name.toUpperCase()}!</h1>

šŸŒ Your Environment

Svelte version ^3.1.0, Parcel version ^1.12.3, plugin version 3.0.0


Thanks in advance!

GeordieP commented 5 years ago

I have the same issue.

Adding the following to package.json (as recommended by this comment) get things compiling and displaying on the page:

  "browserslist": [
    "last 1 chrome versions"
  ],

However, HMR throws an error (browser console):

proxy.js:62 Uncaught TypeError: Cannot read property 'apply' of undefined
    at proxyComponent.self.<computed> [as get] (proxy.js:62)
    at proxyComponent._rerender (proxy.js:185)
    at hot-api.js:43
    at Array.forEach (<anonymous>)
    at reload (hot-api.js:43)
    at Object.eval (eval at hmrApply (hmr-runtime.js:153), <anonymous>:104:11)
    at newRequire (main.1f19ae8e.js:47)
    at hmrAcceptRun (hmr-runtime.js:203)
    at hmr-runtime.js:60
    at Array.forEach (<anonymous>)

Manually refreshing the page works fine, so we can provide the --no-hmr CLI option to Parcel and get rid of that issue, but obviously this isn't ideal as we no longer have HMR.

DeMoorJasper commented 5 years ago

Iā€™m aware of this issue and will try to look into it. If anyone has experience with Svelte compiler or just wants to try to fix this feel free to do so.

Sent with GitHawk

Mouvedia commented 5 years ago

Is there a hack/fix that doesn't involve using browserslist?

madbrain commented 5 years ago

This problem seems to be related to parcel-bundler/parcel#1037 and the source from node_modules packages not being transpiled. Svelte v3 (tested with 3.3.0) contains some ES6 syntax (mostly class declarations). If I understand correctly all the explanations (which I doubt), the correction has to be made on svelte itself :cry:.

Or as @Mouvedia mentioned, use browserslist in order to get only ES6 code and not mix it with transpiled code.

DeMoorJasper commented 5 years ago

@madbrain seems like it's the only way. I've tried fixing it in the plugin a little while ago. Couldn't really figure out a way around this

Mouvedia commented 5 years ago

use browserslist

I don't want to.

If I understand correctly all the explanations (which I doubt), the correction has to be made on svelte itself šŸ˜¢.

@madbrain can you fill it?

madbrain commented 5 years ago

I opened an issue on svelte (sveltejs/svelte#2788).

Hope this would help.

madbrain commented 5 years ago

Well... after some comments on the issue I opened, it seems that browserslist should only be used on applications and not library.

So I spent some more hours of debugging and it seems that the problem is coming from Parcel itself: when an external JS asset is transformed to AST, the minimal amount of babel plugins is computed. But when external module contains no information (browserslist, babelrc, etc.), the target env serves as reference (https://github.com/parcel-bundler/parcel/blob/master/packages/core/parcel-bundler/src/transforms/babel/env.js#L21), which leads to no transpilation at all.

Guys, this is a marathon... :sweat:

DeMoorJasper commented 5 years ago

@madbrain there has actually been various discussions around libraries needing to explicitly mention their target env as otherwise bundlers have no way of knowing which entrypoint to pick/env to start from. There is unfortunately no standard to this as package.json is not really standardised at all.

There have been some proposals and parcel just tries to figure it out based on all semi-standardised fields like browserslist. Not sure why they don't wanna mention it. But they should.

madbrain commented 5 years ago

Is it the desired behaviour to have no transpilation at all when the external module doesn't provide any hint ? I got a pending commit (https://github.com/madbrain/parcel/commit/18f9b1f40de9f8e091dd71b03e80c6312c04ec89) which correct this behaviour, but it sounds to obvious to really be what you (parcel maintainers) got on your mind.

DeMoorJasper commented 5 years ago

@madbrain perhaps, imo this would make more sense. but it would slow down Parcel significantly as packages who don't have a browserslist or something similar will all get compiled (and most packages actually do target commonjs, which makes this prob not such a good idea). Which unfortunately is way too many packages. It would be great if tools like babel actually enforced a similar pattern to Parcel and use browserlist file or package.json instead of allowing defining target env in babel. Or at least not encourage it.

This (having no idea what a package targets problem) was actually mentioned in a talk at Google I/O about how package creators could help improve bundle size and in Parcel's case also Bundler performance by using some target field https://youtu.be/-xZHWK-vHbQ?t=2294 (although I'm no fan of their suggested syntax, as it's a very short term solution...)

Anyway the package.json "spec" still has a long way to go before it can actually help improve bundle size and bundler speed. Luckily a lot of people are trying to push these things to become standard including the Parcel team. (unfortunately also leading to multiple specs)

DeMoorJasper commented 5 years ago

A link to my original PR on this with a bunch of comments why this isn't a good idea and possible solutions: https://github.com/parcel-bundler/parcel/pull/2073

Mouvedia commented 5 years ago

@DeMoorJasper Bower has "moduleType".

orlov-vo commented 5 years ago

I using this temprorary workaround (thx @icopp for comment) for solve the bug. It just trigger a transpilation svelte module via Babel (I hope it will be fixed in next releases of Parcel)

package.json:

{
  "scripts": {
    "postinstall": "cpy --rename=.browserslistrc .browserslistrc.packages node_modules/svelte"
  }
}

.browserslistrc.packages:

last 1 chrome versions
blairn commented 5 years ago

I can't work out what state this is at, does this mean IE 11 + Svelte + Parcel is right out?

DeMoorJasper commented 5 years ago

@blairn you can use the workaround by @orlov-vo

Sent with GitHawk

Mouvedia commented 5 years ago

This is not a workaround. It's a hack that only works if you are not supporting certains browsers.

blairn commented 5 years ago

I couldn't make it work. But I think it is more than just this plugin, or svelte, or anything. I think IE11 support is just a really hard target for anything written in a modern style.

Needless to say, without a better choice, we are moving back to writing code like it is 1999.

samuelgozi commented 5 years ago

This is not an issue specific to this plugin, this is an issue that is caused due to some design decisions made in Parcel. Specifically(as mentioned above) the decision not to transpile packages in the node_modules folder. You can see the issue in their repository here: https://github.com/parcel-bundler/parcel/issues/1655

I'm subscribed to that issue, and to other issues regarding HMR, and will update this plugin when possible. Until then there is nothing we can do... EDIT: Updated the link to the issue, accidentally linked a different one.

lucas-hugdahl commented 4 years ago

@madbrain perhaps, imo this would make more sense. but it would slow down Parcel significantly as packages who don't have a browserslist or something similar will all get compiled (and most packages actually do target commonjs, which makes this prob not such a good idea). Which unfortunately is way too many packages. It would be great if tools like babel actually enforced a similar pattern to Parcel and use browserlist file or package.json instead of allowing defining target env in babel. Or at least not encourage it.

This (having no idea what a package targets problem) was actually mentioned in a talk at Google I/O about how package creators could help improve bundle size and in Parcel's case also Bundler performance by using some target field https://youtu.be/-xZHWK-vHbQ?t=2294 (although I'm no fan of their suggested syntax, as it's a very short term solution...)

Anyway the package.json "spec" still has a long way to go before it can actually help improve bundle size and bundler speed. Luckily a lot of people are trying to push these things to become standard including the Parcel team. (unfortunately also leading to multiple specs)

I really wish there was an option to transpile node_modules. Can Parcel read read node_modules only once and utilize the cache to check if anything has changed? I don't like the idea of assuming all node_modules are already "standard js".

jpray commented 4 years ago

Maybe this has been mentioned before, but babel and typescript both have options to allow transpiling of certain node_modules. If parcel would respect what the app has defined in their babel or typescript config files, then apps would have a better workaround.

example within a tsconfig.json file:

{
    "compilerOptions": {
      "module": "es2015",
      "target": "es5",
      "downlevelIteration": true,
      "allowJs": true,
      ...
    },
    "include": [
        "./*",
        "./node_modules/lit-html/**/*",
        "./node_modules/lit-element/**/*",
      ],
    ...
}
hmmhmmhm commented 4 years ago

I used parcel and svelte for several months, during which I developed a more accurate postinstall script to solve this issue.

// postinstall.js
const fs = require('fs')
const nodeVersion = 'node 10.11'

// Patch to node_modules/*
const patch = (staticPath) => {
    let folderNames = fs.readdirSync(staticPath)
    for(let folderName of folderNames){
        let stats = fs.statSync(staticPath + '/' + folderName)
        if(! stats.isDirectory()) continue

        try{
            let packageFilePath = `${staticPath}/${folderName}/package.json`
            let browserListFilePath = `${staticPath}/${folderName}/.browserslistrc`
            let packageFileData = JSON.parse(fs.readFileSync(packageFilePath))

            delete packageFileData['browserslist']
            fs.writeFileSync(browserListFilePath, nodeVersion)
            fs.writeFileSync(packageFilePath, JSON.stringify(packageFileData, null, 2))
            // console.log(`Fixed browserlist in ${packageFilePath}`)

            // Patch to node_modules/*/node_modules/*
            let nestedModulePath = `${staticPath}/${folderName}/node_modules`
            if(fs.existsSync(nestedModulePath)) patch(nestedModulePath)
        }catch(e) {}
    }
}

patch('./node_modules')
console.log(`All browserlist has been updated.`)

Enter the above code into the project main folder to 'postinstall.js' and add the following script to package.json.

{
  "scripts": {
    "postinstall": "rm -rf ./.cache && node ./postinstall.ts"
  }
}

Now, if you do npm install, the browser list is automatically patched at the end.

CAUTION

Postinstall does not appear to run automatically on manual module installation commands such as npm install <module name>, The npm run postinstall command must be manually executed after you install the manual module install.

plmrry commented 4 years ago

If you don't want to use the browserslist solution, I wrote a Parcel plugin that forces a Babel transform on certain JS assets:

ForceBabel.js

const fs = require('@parcel/fs');
const babel = require('@babel/core');
const JSAsset = require('parcel-bundler/src/assets/JSAsset');

// Only run Babel on Svelte source files
const babelConfig = {
  include: ['node_modules/svelte'],
  plugins: ['@babel/plugin-transform-classes']
};

module.exports = class ForceBabel extends JSAsset {
  constructor(...args) {
    super(...args);
  }
  // This overloads the JSAsset's`load` function.
  // It transforms the JS file immediately after reading it, 
  // before Parcel has done anything.
  async load() {
    const code = await fs.readFile(this.name, this.encoding);
    const transformed = babel.transformSync(code, {
      filename: this.name,
      ...babelConfig
    }).code;
    return transformed;
  }
};

build.js

const Bundler = require('parcel-bundler');

const bundler = new Bundler(`src/js/main.js`);

// This is how you add a plugin to Parcel
const path = require.resolve('./ForceBabel.js');
bundler.addAssetType('js', path);
bundler.addAssetType('mjs', path);

bundler.bundle();

If you want to use the Parcel CLI instead of the Node API, you can put ForceBabel.js in a local directory with a package.json, call it parcel-plugin-force-babel and run npm i file:./parcel-plugin-force-babel .

Mouvedia commented 4 years ago

@plmrry could you create a GitHub repository with a README for this plugin? Remember to enable issues and PRs. This will prevent this very issue from being spammed with questions about your plugin. Thanks.

tcrowe commented 4 years ago

This might be caused by the way babel transforms the svelte output.

For example one of my components outputs this text in the class definition.

class StyleGuide extends SvelteComponentDev {
    constructor(options) {
        super(options);
        init(this, options, instance, create_fragment, safe_not_equal, {});

        dispatch_dev("SvelteRegisterComponent", {
            component: this,
            tagName: "StyleGuide",
            options,
            id: create_fragment.name
        });
    }
}

Babel and webpack turn it into this gobbledegook where this function _classCallCheck throws that error.

var StyleGuide = /*#__PURE__*/function (_SvelteComponentDev) {
  _inherits(StyleGuide, _SvelteComponentDev);

  function StyleGuide(options) {
    var _this;

    _classCallCheck(this, StyleGuide);

    _this = _possibleConstructorReturn(this, _getPrototypeOf(StyleGuide).call(this, options));
    (0,svelte_internal__WEBPACK_IMPORTED_MODULE_15__.init)(_assertThisInitialized(_this), options, instance, create_fragment, svelte_internal__WEBPACK_IMPORTED_MODULE_15__.safe_not_equal, {});
    (0,svelte_internal__WEBPACK_IMPORTED_MODULE_15__.dispatch_dev)("SvelteRegisterComponent", {
      component: _assertThisInitialized(_this),
      tagName: "StyleGuide",
      options: options,
      id: create_fragment.name
    });
    return _this;
  }

So, I made a babel config containing some customizations for @babel/preset-env. npm install core-js and useBuiltIns are probably not necessary. That's just how my setup goes.

const babelrc = {
  presets: [
    [
      "@babel/preset-env",
      {
        corejs: 3,
        targets: {
          browsers: ["defaults"]
        },
        useBuiltIns: "usage",
        /*

        Svelte breaks if this tries to transform the classes. It calls these babel
        methods and then throws.

        + _classCallCheck
        + _getPrototypeOf

        */
        exclude: ["transform-classes"]
      }
    ]
  ]
};

module.exports = babelrc;

It needs a little more investigation to find out if the svelte output or the babel transform are to blame. Based on my work here I don't think it's an error with DeMoorJasper/parcel-plugin-svelte.

Was this helpful?

Do we need to talk to anyone from the svelte team or do you think it's just babel junk?

@edm00se I thought you might need this for your template.

Related links: https://babeljs.io/docs/en/babel-plugin-transform-classes https://stackoverflow.com/questions/36577683/babel-error-class-constructor-foo-cannot-be-invoked-without-new

Mouvedia commented 4 years ago

What's the status of this issue ? It's a blocker for me. I cannot use my .browserslistrc hence Parcel is a no-go.

hmmhmmhm commented 4 years ago

@Mouvedia May be you can try this, https://github.com/hmmhmmhm/svelte-template

Mouvedia commented 4 years ago

That may help someone else, thanks. I need sass/parcel/svelte/babel/eslint, no typescript.

edm00se commented 4 years ago

@Mouvedia I have a starter that might suit your needs. No ts, but modern js transpilation, parcel, svelt 3, sass support in both global import and svelte components. It also has jest for testing.

Mouvedia commented 2 years ago

Was it finally fixed by parcel-bundler/parcel#7399?