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 esm options for Nodejs #1886

Merged
merged 2 commits into from
Nov 15, 2023
Merged

feat: add esm options for Nodejs #1886

merged 2 commits into from
Nov 15, 2023

Conversation

sofisl
Copy link
Contributor

@sofisl sofisl commented Oct 20, 2023

Most of the files are templates, the main changes are:

  • Add an ESM path to generating index.ts (necessary since import paths are changing, and since the index.ts now lives one level lower than previously)
  • Add an ESM option to generate protos in es6 (soon this will be obsolete, since we moved it to the generator, but this is necessary until we remove compileProtos from here)
  • Add an ESM path for templates (mainly, changing .js --> .cjs)

@snippet-bot
Copy link

snippet-bot bot commented Oct 20, 2023

Here is the summary of changes.

You are about to add 26 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@sofisl sofisl changed the title chore: update synthtool to include esm feat: add esm options for Nodejs Oct 20, 2023
@sofisl
Copy link
Contributor Author

sofisl commented Oct 20, 2023

@bcoe, would you mind taking a look whenever you have a minute? Thank you!

@SurferJeffAtGoogle
Copy link
Contributor

In the pull request description, could you describe why these files are changing?

Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

I believe the majority of the node_esm_mono_repo_library files are the same as the non-ESM versions?

Could we consider giving jinja multiple search paths:

loader = FileSystemLoader(["/override/templates", "/default/templates"])

And just add the template files that differ?

If this change could be done without too much refactoring, it would make it much easier to update templates.

Copy link
Contributor

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

Approving, with the suggestion that we refactor to use two template directories, so that we don't need duplicate copies of CONTRIBUTING, LICENSE, .eslintignore, etc.

@sofisl
Copy link
Contributor Author

sofisl commented Nov 15, 2023

Approving, with the suggestion that we refactor to use two template directories, so that we don't need duplicate copies of CONTRIBUTING, LICENSE, .eslintignore, etc.

After looking into this, I'm a bit hesitant to do this because it would require refactoring this function (which renders the templates) to accept a List of template paths, vs. a string. That would affect Python and Java as well.

I've created a bug for this: #1897

@sofisl sofisl merged commit d7b3591 into master Nov 15, 2023
8 checks passed
@sofisl sofisl deleted the esmChanges branch November 15, 2023 01:24
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.

3 participants