-
Notifications
You must be signed in to change notification settings - Fork 73
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR, this is a great start!
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. |
…JS module by rollup.
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
That will change the |
index.js
Outdated
@@ -70,7 +71,7 @@ function getQualifiedJSXName(object) { | |||
getQualifiedJSXName(object.property); | |||
} | |||
|
|||
module.exports = function(options) { | |||
var acornJsx = function(options) { |
There was a problem hiding this comment.
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("..") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
Yeah that makes sense. Note that you couldn't use an 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... |
Okey, all left over comments were removed, and acornJsx is changed to function declaration. But I'm not very familiar with |
Any update on this? |
Thank you both for the hard work!!! |
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
@elevatebart Emm, in this PR, I also changed the test scripts to use |
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'; |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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
I've checked out the source code of
acorn
, so that decided to use the same bundler(rollup
) to generate the ES Module version ofacorn-jsx
. And useindex.mjs
as the filename of the ES Module, just likeacorn-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
inDeno
, you should configure an imports map like below:Because
acorn-jsx
has a propertytokTypes
that relies onacorn
, in the ES Module ofacorn-jsx
it is imported byimport "acorn";
.