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

CommonJS settings cause linked JS file to not load #127

Closed
Phillipus opened this issue Feb 21, 2024 · 18 comments
Closed

CommonJS settings cause linked JS file to not load #127

Phillipus opened this issue Feb 21, 2024 · 18 comments

Comments

@Phillipus
Copy link
Member

Phillipus commented Feb 21, 2024

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
Copy link
Member

jbsarrodie commented Feb 21, 2024 via email

@Phillipus
Copy link
Member Author

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

@Phillipus
Copy link
Member Author

Phillipus commented Feb 21, 2024

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();
}
  • If our polyglot properties are set the second option is chosen, module.exports = factory(); and the objects fail to load
  • If our polyglot properties are not set the third options is chosen, root.Papa = factory(); and the objects load
  • If we replace the second option with root.Papa = factory(); then everything works OK and the objects load

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

@Phillipus
Copy link
Member Author

Phillipus commented Feb 21, 2024

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

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

@jbsarrodie
Copy link
Member

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

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

@jbsarrodie
Copy link
Member

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

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
Copy link
Member

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

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

Done. ;-)

@jbsarrodie
Copy link
Member

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

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
Copy link
Member

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:

  • load() works with local or remote content.
  • require() works with local content only
    • require("moduleName") will look for a module stored under node_modules/moduleName (which is assumed to contain a CommonJS module, which can be as simple as a single index.js file or contain much more than that)
    • require("Path/To/Some/File.js") will load File.js assuming it is a "self contained" CommonJS module

@jbsarrodie
Copy link
Member

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

@Phillipus
Copy link
Member Author

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

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

@Phillipus
Copy link
Member Author

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

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