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

Add support for ES Module #113

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add support for ES Module #113

wants to merge 5 commits into from

Conversation

SaekiRaku
Copy link

I've checked out the source code of acorn, so that decided to use the same bundler(rollup) to generate the ES Module version of acorn-jsx. And use index.mjs as the filename of the ES Module, just like acorn-jsx do.

However, I also noticed that acorn-jsx is zero-dependencies for now. I don't know if it's acceptable to include a new dependency. Actually, the easiest way to do that is RegExp + String replace, but without lexical analysis, it will bring new issues in the future.

At last, if someone wants to use acorn-jsx in Deno, you should configure an imports map like below:

{
    "imports": {
        "acorn": "https://cdn.jsdelivr.net/npm/acorn@7.2.0/dist/acorn.mjs"
    }
}

Because acorn-jsx has a property tokTypes that relies on acorn, in the ES Module of acorn-jsx it is imported by import "acorn";.

@RReverser
Copy link
Member

Thanks for the PR, this is a great start!

However, I also noticed that acorn-jsx is zero-dependencies for now. I don't know if it's acceptable to include a new dependency.

These are devDependencies, and so perfectly fine since they will be installed only by maintainers.

I think what we might want here instead though is to make ES module the primary source, and auto-generate CommonJS instead.

This would make the code that maintainers actually work with use modern syntax, and CommonJS a production artifact for older versions of Node, which also makes it easier to deprecate it altogether in some far future when everyone migrates.

@SaekiRaku
Copy link
Author

I've transformed all codes to ES6 import/export syntax, including the test. And use these to generate a CommonJS module.

But there is a problem with tokTypes, if use export { tokTypes } in the ES Module. Rollup will bundle up like below:

exports.default = acronJsx;
export.tokTypes = tokTypes;

That will change the require syntax for the CommonJS users. So, for now, I'm keeping the Object.defineProperty(acornJsx, "tokTypes", {...}) and commented export { tokTypes }.

index.js Outdated
@@ -70,7 +71,7 @@ function getQualifiedJSXName(object) {
getQualifiedJSXName(object.property);
}

module.exports = function(options) {
var acornJsx = function(options) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a function declaration.

test/run.js Outdated
@@ -18,7 +20,7 @@ function log(title, message) {
if (typeof console === "object") console.log(title, message);
}

const acorn = require("acorn"), jsx = require("..")
// const acorn = require("acorn"), jsx = require("..")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a leftover?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same for few other comments)

@RReverser
Copy link
Member

But there is a problem with tokTypes, if use export { tokTypes } in the ES Module. Rollup will bundle up like below:

Yeah that makes sense. Note that you couldn't use an export { tokTypes } because it's a getter anyway.

However, that property was added mainly for backwards compatibility and the right way to use it is now via the value returned from the factory function. I wonder if, together with this change, we could just get rid of it and make a major bump...

@SaekiRaku
Copy link
Author

SaekiRaku commented Jun 2, 2020

Okey, all left over comments were removed, and acornJsx is changed to function declaration.

But I'm not very familiar with acorn, I just start to using it recently. 😅I'm not quite understanding how tokTypes works.

@Alaboudi1
Copy link

Any update on this?

@elevatebart
Copy link

Thank you both for the hard work!!!
How can I help bring it closer to ready to merge?

index.cjs.js Outdated
@@ -0,0 +1,747 @@
// DO NOT edit this file, it's auto generate from 'constructor/build.js', any changes will be overwrite.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this file be in .gitignore since it is generated?

Copy link
Author

@SaekiRaku SaekiRaku Nov 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have added it to .gitignore.

@SaekiRaku
Copy link
Author

@elevatebart Emm, in this PR, I also changed the test scripts to use import Syntax. So they need esm as devDependencies to run the test scripts. But the travis-ci config didn't install all dependencies, therefore the ci checks have failed.

@thescientist13
Copy link

Any updates available or anything anyone can do to help push this along?

@@ -1,6 +1,7 @@
'use strict';

const XHTMLEntities = require('./xhtml');
import * as acorn from "acorn";
import XHTMLEntities from './xhtml';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this import would need to have an extension?

import XHTMLEntities from './xhtml.js';

@@ -3,6 +3,8 @@
"description": "Modern, fast React.js JSX parser",
"homepage": "https://github.com/acornjs/acorn-jsx",
"version": "5.3.1",
"main": "index.cjs.js",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, I think we would want to add "type": "module" to package.json as well

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.

None yet

5 participants