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

[bug] Rollup: Unexpected character '#' #18

Closed
rennokki opened this issue Mar 29, 2022 · 7 comments
Closed

[bug] Rollup: Unexpected character '#' #18

rennokki opened this issue Mar 29, 2022 · 7 comments

Comments

@rennokki
Copy link
Contributor

rennokki commented Mar 29, 2022

Rollup throws this error:

SyntaxError: Unexpected character '#' (67:2) in [redacted]\node_modules\dog\index.js

It seems to be caused by hashtag-prefixed members in the class, in index.js:

 // src/group.ts
 var Group = class {
  #child; // <------ THIS LINE
  #mapping;
  #kids;
  #sorted;
  #current;
  constructor(state, env) {
    this.uid = state.id.toString();
    this.#mapping = new Map();
    this.#kids = new Map();
    this.#sorted = [];
    let refs = this.link(env);
    this.#child = refs.child;
  }
  receive(req) {
    return abort(404);
  }

Any idea how to make it work with Rollup? I can't seem to find a straightforward solution on Google too. The example from this repo is not bundling the worker through Webpack or Rollup.

@lukeed
Copy link
Contributor

lukeed commented Mar 29, 2022

What is your version of Node? These are private class fields & have been supported since Node 12.4

@rennokki
Copy link
Contributor Author

I got Node 16.x, so it's not the issue.

Rollup seems to have some issues with the TC, even with AcornPrivateFields installed and loaded into the config.

// rollup.config.js
import commonjs from '@rollup/plugin-commonjs';
import injectProcessEnv from 'rollup-plugin-inject-process-env';
import json from '@rollup/plugin-json';
import nodeGlobals from 'rollup-plugin-node-globals'
import nodePolyfills from 'rollup-plugin-node-polyfills';
import { nodeResolve } from '@rollup/plugin-node-resolve';
import { terser } from 'rollup-plugin-terser';
import typescript from '@rollup/plugin-typescript';

export default {
    input: 'src/index.ts',
    output: {
        exports: 'named',
        format: 'es',
        file: 'dist/index.mjs',
        sourcemap: true,
    },
    plugins: [
        typescript(),
        commonjs(),
        json(),
        injectProcessEnv({
            CODE_VERSION: process.env.CODE_VERSION || 'dev',
        }),
        nodeResolve({ browser: true }),
        nodePolyfills({ crypto: true }),
        nodeGlobals({ process: true }),
        terser(),
    ],
}

I also tried to use es2020 instead of esnext, yet nothing got fixed.

{
  "compilerOptions": {
    "target": "esnext",
    "lib": ["esnext"],
    "downlevelIteration": true,
    "moduleResolution": "node",
    "outDir": "dist",
    "esModuleInterop": true,
    "typeRoots": [
      "./node_modules/@types",
      "./node_modules/@cloudflare"
    ],
  },
  "include": ["src"],
  "exclude": ["node_modules", "dist"]
}

@rennokki
Copy link
Contributor Author

Would DOG still function well if the private properties are dropped in favor of private typehinted properties?

class Group {
  // Before
  #children;

  // After
  private children;
}

@lukeed
Copy link
Contributor

lukeed commented Mar 29, 2022

The source isn't changing. It's a stable language feature.

Rollup accepts the syntax just fine: see REPL

It's another part of your toolchain.

@rennokki
Copy link
Contributor Author

Node Globals Rollup plugin is 4-6 years old and is stuck on acorn 5.x

Currently, acorn is 8.x, which has support for all ES2022 features. 🤦🏼‍♂️ And given the fact that npm installs packages for other packages, there was no way to force acorn 8.x

It was indeed my toolchain. Disabling nodeGlobals() from the plugins in Rollup did the job, but now I have to find an alternative.

@lukeed
Copy link
Contributor

lukeed commented Mar 30, 2022

Ah, nice find! I think you should only need rollup-plugin-node-polyfills – although it'd be better to find/update dependencies that don't need Node internals at all!

@rennokki
Copy link
Contributor Author

Can't remember why I needed the nodeGlobals(), perhaps to access the process constant. I didn't actually need it in the project after all. 😅

Anyway, great package. It was easy to implement and wanted to build myself one, now we know who codes faster. 😄

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

No branches or pull requests

2 participants