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

js-interpreter including ESM and in the npm ecosystem #268

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

mercmobily
Copy link

@mercmobily mercmobily commented Mar 24, 2024

Alright, I did it. This is finally something I am actually proud of.
So, here are the changes in a nutshell:

acorn.js

I changed the export code at the top so that it's a little neater, AND it's all ready for the possible tiny code injection at the top to turn this into an ESM. I got rid of AMD support (which would have made things way more difficult).
I got rid of the globalThis pattern. It can be re-added, but... I am not 10000% sure what pattern that is; let me know if it's something that's really absolutely wanted.

(Please note that I am a big fan of keeping things simple and easy. I am sure there are plenty of magic tools out there that are able to turn the given file into a magic UMD. The tool you use depends on the current fashion; I've seen so many... Grunt, Gulp, Webpack, Rollup, Parcel... then the current fashion is Vite but then there is BUN... Sorry, I prefer 9 lines of code, especially now that we can FINALLY say that AMD is history. Hopefully, CJS and "browser globals" will also be history soon enough, and we will no longer need any building, etc. Till then, it's 9 lines of code...)

So, here is a summary of the changes:

interpreter.js

I added the header to make it multi-format (commonJS, browser's global, and ESM if a tiny injection at the top is added).
It now uses a well established multi-format pattern. So, I got rid of the Interpreter.nativeGlobal variable which was based on globalThis etc. Another thing I did -- partially as a consequence of the nativeGlobal thing` was to change this:

    value = arguments.length ? Interpreter.nativeGlobal.String(value) : '';

Into this:

    value = arguments.length ? String(value) : '';

EDIT: No I didn't. I made this change, and then fixed it when I realised that the point is to use the original methods before redefining them. I did it but without using the now removed nativeGlobal -- see change to see**

The same for Boolean, Number, Date, etc.
I am still not sure what globalThis is about. I looked at the issues, and there is something about problems with using import on it (which I am AMAZED it would ever even work). But it looks like it's something that is addresses with this PR.

createEsmFiles.js

This is a new file. It injects TINY amounts of code into interpreter.js and acorn.js so that they export the right variables. This building step is necessary.
The added code is very minimal.
This is added to interpreter.js:

import { parse, version } from './acorn-esm.js';
var asEsm = true;
export { Interpreter, parse, version as acornVersion };

And this is added to acorn.js:

export { parse, version };
var asEsm = true;

That's it! This will create interpreter-esm.js and acorn-esm.js as fully usable esm modules. Because the step is so tiny, I actually do this as a postinstall task in NPM.

This could potentially be a problem as people will only get those files after a npm install. If they clone it, they will need to run generateEsmFiles.js to generate them.

Alternatively, we can turn this into a build task, and remove the -esm files from .gitignore.

index-esm.html

This is the same as index.html, but it loads the interpreter as an ESM module instead. It works exactly the same.

We need to decide what to do about the acorn_interpreter.js file. I am sure we need to keep it, or existing users that expect it will have a heart attack. I THINK the building process should work the exact same (or maybe THAT is the piece of the puzzle that needs nativeGlobal?!?).

I need to test this in a proper NPM project, to check that tools like vite load everything fine etc. But I don't anticipate huge problems there.

I hope this helps.

@mercmobily
Copy link
Author

I did some extra testing, discovered why it was important ti have Interpreter.nativeGlobal.Number() and not just Number()... My apologies, it should have been obvious. I did extra testing, and things seem to work now

@mercmobily
Copy link
Author

Is this something you are considering?

@Kreijstal
Copy link

maybe you will be the new mantainer lol

@mercmobily
Copy link
Author

mercmobily commented Apr 10, 2024 via email

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

Successfully merging this pull request may close these issues.

2 participants