archimatetool / archi-scripting-plugin

jArchi - Scripting for Archi: ArchiMate Modelling Tool
https://www.archimatetool.com
118 stars 33 forks source link

CommonJS settings cause linked JS file to not load #127

Closed Phillipus closed 4 months ago

Phillipus commented 5 months ago

Since jArchi 1.6 we have two new properties:

System.setProperty("polyglot.js.commonjs-require", "true");
System.setProperty("polyglot.js.commonjs-require-cwd", ArchiScriptPlugin.INSTANCE.getPreferenceStore().getString(IPreferenceConstants.PREFS_SCRIPTS_FOLDER));

However, some imported JS files no longer work.

  1. Add papaparse.js to a lib folder in your jArchi scripts folder papaparse.js.zip
  2. Run this simple script:
    console.show();
    console.clear();
    load(__DIR__ + "lib/papaparse.js");
    console.log("> Loaded Papa Parse");
    console.log(Papa);
> Loaded Papa Parse
org.graalvm.polyglot.PolyglotException: ReferenceError: "Papa" is not defined

@jbsarrodie WDYT?

(PapaParse source - https://www.papaparse.com/)

papaparse.js.zip

jbsarrodie commented 5 months ago

I'll look at it tonight.

JB

Phillipus commented 5 months ago

For reference here's the source of papaparse.js - papaparse.js.zip

Phillipus commented 5 months ago

Here's the first few lines of papaparse.js:

/* globals define */
if (typeof define === 'function' && define.amd)
{
    // AMD. Register as an anonymous module.
    define([], factory);
}
else if (typeof module === 'object' && typeof exports !== 'undefined')
{
    // Node. Does not work with strict CommonJS, but
    // only CommonJS-like environments that support module.exports,
    // like Node.
    module.exports = factory();
}
else
{
    // Browser globals (root is window)
    root.Papa = factory();
}

So, something to do with calling module.exports = factory(); instead of root.Papa = factory();

Phillipus commented 5 months ago

This papaparse.js file needs to be treated like a CommonJS file.

  1. Create a subfolder node_modules/papaparse
  2. Copy papaparse.js into that folder and rename it to index.js
  3. Replace load(__DIR__ + "lib/papaparse.js"); with const Papa = require("papaparse");
  4. It works

@jbsarrodie How to manage this situation? Add a jArchi preference option to enable/disable CommonJS? But then that would be a global setting. It's not possible to set those polyglot properties in a jArchi script because they have to be set before the GraalVM engine starts and can't be unset.

Phillipus commented 4 months ago

I've added an option in preferences to enable/disable CommonJS in the dev branch.

jbsarrodie commented 4 months ago

This papaparse.js file needs to be treated like a CommonJS file.

Thank you for your analysis, you're right. In fact, Papaparse authors tried to make their lib "intelligent", and when the code is loaded, it tries to figure out in which context (browser, node.js or other) it has been executed. Unfortunately, there is no perfect method for that and their approach has some side effects in this case. So this is not an issue with jArchi itself, but a side effect of how this very specific javascript lib has been coded. And that's the reason why I didn't experienced this issue during my tests (I'm using several other libs with no issues, and I didn't tested the one script I have which relies on Papaparse).

@jbsarrodie How to manage this situation?

I think the best option is to document our findings in a wiki page and update the Changelog to make sure people facing the issue understand it and find the quick solution.

Speaking about the solution...

  • Create a subfolder node_modules/papaparse
  • Copy papaparse.js into that folder and rename it to index.js
  • Replace load(__DIR__ + "lib/papaparse.js"); with const Papa = require("papaparse");
  • It works

There is an easier fix: require() can load modules using their absolute path. So no need for all these steps, simply replace

load(__DIR__ + "lib/papaparse.js");

by

const Papa = require(__DIR__ + "lib/papaparse.js");

I've added an option in preferences to enable/disable CommonJS in the dev branch.

IMHO, we don't need to add such option only to solve an issue related to the way a specific lib has been written. I'd better keep jArchi 1.6 as it is and let people change one line their code.

Phillipus commented 4 months ago

IMHO, we don't need to add such option only to solve an issue related to the way a specific lib has been written. I'd better keep jArchi 1.6 as it is and let people change one line their code.

OK, but I don't think there's any harm in adding the option just in case a show-stopper is found and to help diagnose problems.

Phillipus commented 4 months ago

I updated https://github.com/archimatetool/archi-scripting-plugin/wiki/Using-Node.js-modules

jbsarrodie commented 4 months ago

OK, but I don't think there's any harm in adding the option just in case a show-stopper is found and to help diagnose problems.

I don't know. I'm not so much in favor of adding options without a strong need, and I don't see this Papaparse issue as a real one. Of course that could be annoying, but users who have the knowledge to write advanced jArchi scripts relying on external library should be able to fix this in less than a minute, And, as you said such option is a global setting. Furthermore, this option is only meaningful with GraalVM, so we would need additional code to make sure it is grayed out if another script engine is set.

Instead of an option in preferences, could it be simply an argument we add to the command line to disable module support? In the same vein as -noModelCheck, it could be -noCommonJS.

Phillipus commented 4 months ago

so we would need additional code to make sure it is grayed out if another script engine is set.

It doesn't do any harm if not greyed out as the setting is ignored for Nashorn.

Instead of an option in preferences

I do prefer to have an easily accessible option for now. It will help me when I have to deal with requests for support. ;-)

jbsarrodie commented 4 months ago

I do prefer to have an easily accessible option for now. It will help me when I have to deal with requests for support. ;-)

Ok, I do understand. But let's make sure CommonJS is enabled by default, and if needed, people will have to opt-out.

Phillipus commented 4 months ago

But let's make sure CommonJS is enabled by default, and if needed, people will have to opt-out.

Done. ;-)

jbsarrodie commented 4 months ago

I updated https://github.com/archimatetool/archi-scripting-plugin/wiki/Using-Node.js-modules

I don't think this work with require() (I did some tests and it never did for me)

const myVar = require('https://web-module.location');

Have you been able to make it work ?

Phillipus commented 4 months ago

Have you been able to make it work ?

I didn't test it, I added it because I found some documentation on-line. ;-)

I've removed that now.

jbsarrodie commented 4 months ago

I didn't test it, I added it because I found some documentation on-line. ;-)

Never trust online documentation ;-)

Short summary of what I found ad tested:

jbsarrodie commented 4 months ago

And BTW, I'll update the wiki page with more information soon too.

Phillipus commented 4 months ago

And BTW, I'll update the wiki page with more information soon too.

Thanks! I'm out of my depth on this one. ;-)

Phillipus commented 4 months ago

I'll close this issue. I've updated the wiki with more info and jArchi 1.6.1 has the enable/disable CommonJS option.