Skip to content

Fixes #1230, adding ES Module output #1621

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

benjamind
Copy link

I took a stab at adding an ES Module build output using a Rollup build process similar to the existing browserify one.

I think this should work but will likely need some further testing to be sure as I haven't yet used this in a project.

While this PR does not touch any of the existing build outputs, it does add an exports field to the package.json to map the imports to either the existing CJS or the new ESM builds. This change can be a breaking change since it effectively isolates the interface to the package in modern resolvers, I believe I have however mapped all the existing intended exports of this package using this field but should probably get confirmation of this also.

@@ -300,9 +300,6 @@ export class FieldBase extends ReflectionObject {
*/
constructor(name: string, id: number, type: string, rule?: (string|{ [k: string]: any }), extend?: (string|{ [k: string]: any }), options?: { [k: string]: any }, comment?: string);

/** Field rule, if any. */
public rule?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why the .d.ts file is changed in this PR, is it intended?

Copy link
Author

Choose a reason for hiding this comment

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

Seems to be a consequence of running build again, I suspect its been changed in some other commit but the project hasn't been fully rebuilt since?

@alexander-fenster alexander-fenster requested a review from bcoe June 14, 2021 22:41
@vasco-santos
Copy link

@bcoe @alexander-fenster what is the state of this PR? Do you intend to land this?

@steinybot
Copy link
Contributor

This is missing the cli exports.

I'm trying these changes with https://github.com/steinybot/protobufjs-webpack-plugin and it fails with:

Error [ERR_PACKAGE_PATH_NOT_EXPORTED]: Package subpath './cli/pbjs' is not defined by "exports" in /Users/jason/src/goodcover/core/e2e/node_modules/protobufjs/package.json

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.

4 participants