lapo-luchini / asn1js

JavaScript generic ASN.1 parser
ISC License
586 stars 161 forks source link

Bundling with Rollup is not working #53

Closed hodossy closed 2 years ago

hodossy commented 2 years ago

It is stated in the documentation that bundling with a standard JS bundler is straightforward, however I find it hard to make Rollup work. Could you show an example of such a configuration with any major bundler? Thanks for the efforts!

piotrwitek commented 2 years ago

Hey @hodossy, I also need this for Parcel as it's using rollup behind the scenes and I found the culprit of this issue and how to fix it. I will try to make a fork tomorrow with the fix and open a PR.

lapo-luchini commented 2 years ago

This ES test works:

% cat index.mjs
import ASN1 from '@lapo/asn1js';
import Hex from '@lapo/asn1js/hex.js';
console.log(ASN1.decode(Hex.decode('06032B6570')).content());
% node index.mjs
1.3.101.112
curveEd25519
EdDSA 25519 signature algorithm

…but it does indeed not work with rollup, which complains about default not being exported.

I'm no expert of rollup unfortunately, it probably doesn't like the ES/CommonJS/AMD boilerplate I'm using.

piotrwitek commented 2 years ago

@lapo-luchini, thanks for chiming in. I believe here is the issue, and after testing, by moving "require" statements outside the conditions to the top scope it works as expected with rollup.

Avoid conditional require() [ref](https://parceljs.org/features/scope-hoisting/#avoid-conditional-require()) Unlike ES module import statements which are only allowed at the top-level of a module, require is a function that may be called from anywhere. However, when require is called from within a conditional or another control flow statement, Parcel must wrap the resolved module in a function so that side effects are executed at the right time. This also applies recursively to any dependencies of the resolved module.

// 🚫 Conditional requires cause recursive wrapping if (someCondition) { require('./something'); }

lapo-luchini commented 2 years ago

I see. Current boilerplate is used to be compatibile with CommonJS (require), and ES (import) and RequireJS at the same time (with the same code)… but seems to be a bad idea for Rollup. At the moment I don't see a solution to get the 4 options with the same code. I'm trying with @rollup/plugin-commonjs + dynamicRequireTargets but so far I didn't manage to get a working bundle.

lapo-luchini commented 2 years ago

I managed to have it working with the following rollup.config.js:

import path from 'path';
import commonjs from '@rollup/plugin-commonjs';
import alias from '@rollup/plugin-alias';

export default {
    input: 'index.mjs',
    output: {
        dir: 'output',
        name: 'testRollup',
        format: 'umd',
    },
    plugins: [
        alias({
            entries: [
                { find: /@lapo[/]asn1js[/]([a-z0-9]+)(?:.js)?/, replacement: path.resolve(__dirname, 'node_modules/@lapo/asn1js/$1.js') },
                { find: '@lapo/asn1js', replacement: path.resolve(__dirname, 'node_modules/@lapo/asn1js/asn1.js') },
            ],
        }),
        commonjs({
            dynamicRequireTargets: [
                'node_modules/@lapo/asn1js/*.js',
            ]
        }),
    ],
};

I'm not sure if this is "nice enough" for common rollup usage (I never used it myself, before today).

hodossy commented 2 years ago

Sorry for being this late with the answer, but in the end I managed to get it working with this suggested configuration! I really appreciate the time and effort you have invested in this one, thank you!

lapo-luchini commented 2 years ago

I'll check in the future if there's a better way. At some point I guess I'll do a 2.x release when I use native ES modules and will remove AMD boilerplate, at that point it will probably work out of the box.