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(compartment-mapper): CommonJS with lexical analysis #666

Merged
merged 3 commits into from
Apr 22, 2021
Merged

Conversation

kriskowal
Copy link
Member

This change reintroduces support for CommonJS modules, but using a lexical analyzer instead of a full-parser analyzer. This reuses the analyzer built by @guybedford for Node.js, but including the import graph analysis by @developit in nodejs/cjs-module-lexer#10.

Fixes #501
Refs #655

The change consists of three parts.

  1. a breaking change to SES changes the API for non-ESM static module records such that they are required to statically provide a list of their exports. This in turn enables third-party modules to participate fully in named imports and for their exports to be reexported by intermediate ESMs,
  2. the creation of a fork of the CommonJS module lexer,
  3. the integration of the lexer in the Compartment Mapper. This in turn consists of reverting the prior change that disabled CommonJS support because the parser was too bulky of a dependency, and then replacing and updating the lexical analysis. Notably, the tests do not change.

Individual commits are reviewable. If requested, I can arrange for this change to be distributed into three or more smaller reviews. This is currently proposed as draft so review can begin while I increase the test coverage over over the lexer.

@kriskowal kriskowal requested a review from kumavis April 13, 2021 03:29
@kriskowal kriskowal force-pushed the 655-cjs branch 2 times, most recently from d507e26 to d1a7f35 Compare April 13, 2021 19:13
@kriskowal
Copy link
Member Author

Tests pass and working end to end. Will follow-up with more commits to increase test coverage and catch up with changes to the original lexer that have landed since the PR I forked from.

@kriskowal kriskowal marked this pull request as ready for review April 13, 2021 21:56
@kriskowal kriskowal requested a review from michaelfig April 13, 2021 21:56
@kriskowal kriskowal requested a review from erights April 14, 2021 22:41
@erights
Copy link
Contributor

erights commented Apr 14, 2021

If requested, I can arrange for this change to be distributed into three or more smaller reviews.

@kriskowal , great to see this! But yes, please do break it up. Thanks.

@kriskowal
Copy link
Member Author

This change is now reduced to the last of three parts, a small adjustment to the compartment mapper that finishes the job. I choose you @michaelfig.

@kriskowal kriskowal removed request for erights and kumavis April 22, 2021 21:23
@kriskowal kriskowal assigned michaelfig and unassigned erights Apr 22, 2021
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

LGTM! Just out of curiosity, is the case where MJS uses the CJS affordances handled?

Example, trivially:

import something from './other';
console.log(__dirname, something);
export const foo = require('soso');

That's a feature we use in some of our -resm packages, though not as trivially.

@kriskowal
Copy link
Member Author

LGTM! Just out of curiosity, is the case where MJS uses the CJS affordances handled?

Example, trivially:

import something from './other';
console.log(__dirname, something);
export const foo = require('soso');

That's a feature we use in some of our -resm packages, though not as trivially.

The MJS module type does not support that particular misfeature since it’s not compatible with Node.js (creates an expectation that it ought to work though it won’t). This is very tricky. On the one hand, we could introduce a resm module type that differs only in that this misfeature is supported, but while it does create a bridge from RESM to Endo, it leaves no bridge to Node.js. I’ll mull on this.

@kriskowal kriskowal merged commit 9727329 into master Apr 22, 2021
@kriskowal kriskowal deleted the 655-cjs branch April 22, 2021 21:55
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.

Restore CommonJS to CompartmentMapper
3 participants