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

feat: Add ES6 module export #680

Merged
merged 5 commits into from Dec 23, 2021
Merged

feat: Add ES6 module export #680

merged 5 commits into from Dec 23, 2021

Conversation

spocke
Copy link
Contributor

@spocke spocke commented Dec 16, 2021

This PR adds es2015 module support for exports it also compiles those modules in ES2019 syntax format.

I moved the types to a separate file since that made it easier to import them all and re-export them I changed the index so that it's more explicit what functions are being exported.

Had to add roots to jest because it was trying to run the es6 modules though node and that was breaking not sure why not that familiar with jest.

I also added sideEffects: false to the package.json since this module doesn't have any sideeffects from importing a module that I can see so that should reduce the bundle size if you only use the parser for example.

@fb55
Copy link
Owner

fb55 commented Dec 17, 2021

Hi @spocke, thanks for this great PR!

This will already be a breaking change, but I am trying to keep upgrading as simple as possible. That means that I'd like to avoid any changes that aren't necessary for the ES modules conversion. Could you revert eg. removing default exports?

@spocke
Copy link
Contributor Author

spocke commented Dec 17, 2021

Hi @spocke, thanks for this great PR!

This will already be a breaking change, but I am trying to keep upgrading as simple as possible. That means that I'd like to avoid any changes that aren't necessary for the ES modules conversion. Could you revert eg. removing default exports?

It pretty much compiles to the same thing this is what the default exports index.js looks like:

"use strict";
var __createBinding = (this && this.__createBinding) || (Object.create ? (function(o, m, k, k2) {
    if (k2 === undefined) k2 = k;
    Object.defineProperty(o, k2, { enumerable: true, get: function() { return m[k]; } });
}) : (function(o, m, k, k2) {
    if (k2 === undefined) k2 = k;
    o[k2] = m[k];
}));
var __exportStar = (this && this.__exportStar) || function(m, exports) {
    for (var p in m) if (p !== "default" && !Object.prototype.hasOwnProperty.call(exports, p)) __createBinding(exports, m, p);
};
var __importDefault = (this && this.__importDefault) || function (mod) {
    return (mod && mod.__esModule) ? mod : { "default": mod };
};
Object.defineProperty(exports, "__esModule", { value: true });
exports.stringify = exports.parse = void 0;
__exportStar(require("./parse"), exports);
var parse_1 = require("./parse");
Object.defineProperty(exports, "parse", { enumerable: true, get: function () { return __importDefault(parse_1).default; } });
var stringify_1 = require("./stringify");
Object.defineProperty(exports, "stringify", { enumerable: true, get: function () { return __importDefault(stringify_1).default; } });

This is what the new index.js file looks like:

"use strict";
var __createBinding = (this && this.__createBinding) || (Object.create ? (function(o, m, k, k2) {
    if (k2 === undefined) k2 = k;
    Object.defineProperty(o, k2, { enumerable: true, get: function() { return m[k]; } });
}) : (function(o, m, k, k2) {
    if (k2 === undefined) k2 = k;
    o[k2] = m[k];
}));
var __exportStar = (this && this.__exportStar) || function(m, exports) {
    for (var p in m) if (p !== "default" && !Object.prototype.hasOwnProperty.call(exports, p)) __createBinding(exports, m, p);
};
Object.defineProperty(exports, "__esModule", { value: true });
exports.stringify = exports.parse = exports.isTraversal = void 0;
__exportStar(require("./types"), exports);
var parse_1 = require("./parse");
Object.defineProperty(exports, "isTraversal", { enumerable: true, get: function () { return parse_1.isTraversal; } });
Object.defineProperty(exports, "parse", { enumerable: true, get: function () { return parse_1.parse; } });
var stringify_1 = require("./stringify");
Object.defineProperty(exports, "stringify", { enumerable: true, get: function () { return stringify_1.stringify; } });

Unfortunately rolling it back doesn't work for our use case. We use this with node and .mjs files and there the modules doesn't support the default property at least I couldn't get it to run without getting undefined functions. Also not sure how bundlers deal with export default and tree shaking when the default is a object they tend to not tree shake since it's then a mutable thing so much harder to track.

I could add export default parse; and export default stringify; to both modules if they are imported directly however that would mean that the default will be a function and have a parse property as well for the parse.js module.

@spocke
Copy link
Contributor Author

spocke commented Dec 17, 2021

Follow up on that mjs issue to reproduce that issue if you use:

import { parse } from 'css-what'

console.log(parse("foo"))

With the default exports approach my node 16 throws this error:
SyntaxError: Named export 'parse' not found. The requested module 'css-what' is a CommonJS module, which may not support all module.exports as named exports.

However with the version in this PR it doesn't produce that error. However the mixed default and direct exports seems to not produce any errors in mjs files so I guess it just favors the es6 modules.

@TheSpyder
Copy link

As far as I can tell nobody imports parse.ts and stringify.ts directly, so what they export should make no difference? The index file was already renaming the default function to parse. I don't see how this is a breaking change. If it was, the tests would need to change as well.

@@ -1,3 +1,3 @@
export * from "./parse";
export { default as parse } from "./parse";
Copy link
Owner

Choose a reason for hiding this comment

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

Won't imports have to be updated to include a .js extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically yes in order to get these to work in a browser it needs to have an extension. However both eslink-pluign-node and jest breaks if I change this to have .js extensions.

There is a long thread here:
microsoft/TypeScript#16577

Microsoft don't want to convert these paths to js paths on the fly but they seem to support them now however the rest of the tools for linting and testing here doesn't seem to suppot that. There are workaround for this that for example transpile the js files before being pushed to npm. I've seen people having scripts that just regexp replace this in source. It all depends on how much one want it to work with js modules in the browser. Bundlers will look for default file extensions so this is only an issue if you are loading scripts with <script type="module" src="main.js"></script>.

@@ -45,7 +48,10 @@
},
"license": "BSD-2-Clause",
"jest": {
"preset": "ts-jest"
"preset": "ts-jest",
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a way to test the ESM version of the module as well? Eg. if a __dirname was missed somewhere — would that be caught?

"target": "ES2019",
"module": "es2015",
"outDir": "lib/es",
"moduleResolution": "node"
Copy link
Owner

Choose a reason for hiding this comment

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

Does moduleResolution have to be set here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you don't specify node it will classic if you change module to es2015.
Described here: https://www.typescriptlang.org/docs/handbook/module-resolution.html#module-resolution-strategies

src/parse.ts Outdated Show resolved Hide resolved
src/stringify.ts Outdated Show resolved Hide resolved
@fb55
Copy link
Owner

fb55 commented Dec 20, 2021

Left some comments. Happy to go forward with this, I'm just trying to understand possible gotchas.

@fb55 fb55 changed the title added es6 module export feat: Add ES6 module export Dec 23, 2021
@fb55 fb55 enabled auto-merge (squash) December 23, 2021 12:18
@fb55 fb55 merged commit b3e3d5c into fb55:master Dec 23, 2021
@fb55
Copy link
Owner

fb55 commented Dec 23, 2021

Thanks @spocke! Using this as an opportunity to modernize the library a bit more, will get a new release out soon.

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

3 participants