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

Remove assert import in favour of webpack #1254

Closed
cre8 opened this issue Sep 24, 2023 · 8 comments · Fixed by #1315
Closed

Remove assert import in favour of webpack #1254

cre8 opened this issue Sep 24, 2023 · 8 comments · Fixed by #1315
Labels
bug Something isn't working

Comments

@cre8
Copy link
Contributor

cre8 commented Sep 24, 2023

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.

@cre8 cre8 added the bug Something isn't working label Sep 24, 2023
@mirceanis
Copy link
Member

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
Copy link
Contributor Author

cre8 commented Sep 24, 2023

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
Copy link
Contributor Author

cre8 commented Oct 14, 2023

@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
Copy link
Contributor

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
Copy link
Contributor Author

cre8 commented Nov 10, 2023

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
Copy link
Contributor Author

cre8 commented Jan 3, 2024

@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": {
  1. 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
Copy link
Member

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
Copy link
Contributor Author

cre8 commented Jan 3, 2024

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 :(

cre8 added a commit to cre8/veramo that referenced this issue Jan 17, 2024
Fixes decentralized-identity#1254

Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
cre8 added a commit to cre8/veramo that referenced this issue Jan 19, 2024
Fixes decentralized-identity#1254

Signed-off-by: Mirko Mollik <mirko.mollik@fit.fraunhofer.de>
mirceanis pushed a commit that referenced this issue Jan 24, 2024
fixes #1254

BREAKING CHANGE: The `plugin.schema.json` files are now generated as `plugin.schema.ts`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants