decentralized-identity / veramo

A JavaScript Framework for Verifiable Data
https://veramo.io
Apache License 2.0
431 stars 131 forks source link

Remove assert import in favour of webpack #1254

Closed cre8 closed 8 months ago

cre8 commented 1 year ago

Bug severity 4

Describe the bug When integrating some basic plugins for veramo, I get the following error:

./node_modules/@veramo/message-handler/build/message-handler.js - Error: Module build failed (from ./node_modules/@angular-devkit/build-angular/src/tools/babel/webpack-loader.js):
SyntaxError: /home/mimo/Projects/ssi/node_modules/@veramo/message-handler/build/message-handler.js: Support for the experimental syntax 'importAttributes' isn't currently enabled (2:66):

  1 | import { CoreEvents, } from '@veramo/core-types';
> 2 | import schema from '@veramo/core-types/build/plugin.schema.json' assert { type: 'json' };
    |                                                                  ^
  3 | import { Message } from './message.js';
  4 | import Debug from 'debug';
  5 | const debug = Debug('veramo:message-handler');

Add @babel/plugin-syntax-import-attributes (https://github.com/babel/babel/tree/main/packages/babel-plugin-syntax-import-attributes) to the 'plugins' section of your Babel config to enable parsing.

Since I am using Angular that is based on webpack I found no way to deal with this problem. Adding a custom webpack and setting the babel loader with the mentioned plugin does not seem to work:

module.exports = {
    module: {
        rules: [
          {
            test: /\.m?js$/,
            exclude: /node_modules\/(?!(@veramo)\/).*/,
            use: {
              loader: 'babel-loader',
              options: {
                presets: ['@babel/preset-react', '@babel/preset-env'],
                plugins: ['@babel/plugin-transform-runtime'],
              },
            },
          },
        ],
      },    
}

When I manually removed the assert { type: 'json' }; from all the files it ran without the errors and I could not see any side effects.

Right now I am using a script that runs after the installation of all packages and removes the assertions:

const fs = require('fs');
const path = require('path');

// Recursive function to process files in directories and subdirectories
function processDirectory(directory) {
    const files = fs.readdirSync(directory);

    for (const file of files) {
        const filePath = path.join(directory, file);
        const stats = fs.statSync(filePath);

        if (stats.isDirectory()) {
            processDirectory(filePath);  // Recursive call
        } else {
            processFile(filePath);
        }
    }
}

function processFile(filePath) {
    let content = fs.readFileSync(filePath, 'utf8');
    const pattern = /assert { type: 'json' };/g;

    if (pattern.test(content)) {
        console.log(`Processing file: ${filePath}`);
        content = content.replace(pattern, '');
        fs.writeFileSync(filePath, content, 'utf8');
    }
}

const rootDirectory = path.join('node_modules', '@veramo');  // Assuming the script is executed at the parent directory of @veramo
processDirectory(rootDirectory);

So my question is can we remove the assert feature in favour to be webpack compatible?

To Reproduce Steps to reproduce the behaviour: Add the veramo agent to an angular application with the did-jwt-vc plugin

Observed behaviour Having this feature in the core libraries makes it impossible to use them in non babel environments.

mirceanis commented 1 year ago

Did you try to add the suggested plugin? Your error message is suggesting to Add @babel/plugin-syntax-import-attributes

We use @babel/plugin-syntax-import-assertions in our react-native guide. You could also try to use that.

I agree with you that these assertions are ugly and should be unnecessary, but they seem to be a quirk of the present day JS/TS world. If we removed them, other things would break 🫤 🤕

cre8 commented 1 year ago

I found no way how to add it successfully into the webpack config (there are ways to point to a custom webpack config and then add babel as a module). Most plugins are designed to format the code in the source folder, not in the node_modules one. With the step from the script I was able to resolve it, but I honestly don't like the solution.

According to the docs assertion help the compiler, but since the imported data aren't that huge I would suggest we remove them

cre8 commented 11 months ago

@mirceanis I tried different ways out. When modifying the code with

import { readdirSync, statSync, readFileSync, writeFileSync } from 'fs';
import { join } from 'path';

// Recursive function to process files in directories and subdirectories
function processDirectory(directory) {
    const files = readdirSync(directory);

    for (const file of files) {
        const filePath = join(directory, file);
        const stats = statSync(filePath);

        if (stats.isDirectory()) {
            processDirectory(filePath);  // Recursive call
        } else {
            processFile(filePath);
        }
    }
}

function processFile(filePath) {
    let content = readFileSync(filePath, 'utf8');
    const pattern = /assert { type: 'json' };/g;

    if (pattern.test(content)) {
        console.log(`Processing file: ${filePath}`);
        content = content.replace(pattern, '');
        writeFileSync(filePath, content, 'utf8');
    }
}

const rootDirectory = join('node_modules', '@veramo');  // Assuming the script is executed at the parent directory of @veramo
processDirectory(rootDirectory);

it works. But in my case I am working in a monorepo and the backend services will throw an error:

TypeError: Module "file:///home/mimo/Projects/ssi/node_modules/@veramo/core-types/build/plugin.schema.json" needs an import assertion of type "json"

One suggestion would be to place the content inside a variable and then import it. In this case we already have it as a object and do not need any assert method. In case I find a solution with webpack like the existing one with babel this could also work without changing anything to the code

rkreutzer commented 10 months ago

I'm having the same issue with an ionic/angular app. Your workaround of deleting the assert portion of those statements worked, so thanks.

Even though this didn't work, this is where I got with the babel.config.js file:

module.exports = {
  presets: [
    [
      '@babel/preset-typescript',
      '@babel/preset-env',
      {
        'targets': {
          'node': 'current'
        }
      }
    ]
  ],
  plugins: [
    [
      '@babel/plugin-syntax-import-attributes',
      { 'deprecatedAssertSyntax': true }
    ],
    '@babel/plugin-syntax-import-assertions'
  ]
};

It doesn't appear that my build process looked at this file, so maybe it needs to go into @angular-devkit. Anyway, maybe this will trigger a thought on your end.

cre8 commented 10 months ago

In the meantime I used "postinstall": "patch-package", and created the patches. This seems to be a nice solution in case you don't have any other projects in the monorepo that rely on the feature.

The best solution in my mind would be to move the json object into a ts file and export it as a variable. With this approach we can include it in all frameworks independent of some typescript features.

To break no other third party dependents that rely on the json file, I would suggest to ADD the ts file also in the folder and update the core libs of veramo where we see the errors right now. Of course we need to check if the two files are in sync all the time. (this could be guaranteed with some kind of unit test before we publish it)

cre8 commented 9 months ago

@mirceanis I found another solution that won't break current functionality:

  1. copy the content of the plugin.schema.json and create a .js of it that will export it in the same folder like:
    export default schema = {
    "IResolver": {
        "components": {
            "schemas": {
  2. append the package.json export with the generated .js file:
    "exports": {
    ".": {
      "types": "./build/index.d.ts",
      "require": "./build/index.js",
      "import": "./build/index.js"
    },
    "./build/plugin.schema.json": "./build/plugin.schema.json",
    "./build/plugin.schema": "./build/plugin.schema.js"
    },
    1. update all import schema from '@veramo/core-types/build/plugin.schema.json' assert { type: 'json' }; with import schema from '@veramo/core-types/build/plugin.schema';

This approach will work in all environments since it's a native import without any new functions like typed imports that are not fully supported in the different bundlers. It will also not break current implementation, so it's more a feature than a breaking change :)

When all bundlers are supporting this, we can go back to the assert approach for optimization.

The only downside is an increased package by 254KB since we included the same object twice (as json and as js object). But in my opinion this tradeoff is worth it. We could even minify the object and the json to remove white space.

mirceanis commented 9 months ago

Plugin schemas are (mostly) auto-generated. We can replace the generation scripts to output js files directly instead of json and this should avoid duplication.

This would be a breaking change as it affects the core logic.

Next, we'd have to replace the imports/exports of the JSON-LD contexts to similarly load them from .js files instead of .json(ld), although, in this case we need to ensure that contexts can still be loaded asynchronously from file URLs as the default ones are quite heavy and might not be needed by everyone.

cre8 commented 9 months ago

I am not a fan of a breaking change... so maybe in the v5 we can add the extra file and maybe in v6 we can remove the json approach? Since we are using the assert approach it's not possible to mark the json function as deprecated like you can do it with functions, classes, etc in JS or TS :(