Skip to content

fix(builtin): convert pkg_web to use cjs instead of js - #3500

Merged
gregmagolan merged 1 commit into
bazel-contrib:stablefrom
KrauseStefan:fix-builtin--convert-pkg_web-to-use-cjs-instead-of-js
Feb 3, 2023
Merged

fix(builtin): convert pkg_web to use cjs instead of js#3500
gregmagolan merged 1 commit into
bazel-contrib:stablefrom
KrauseStefan:fix-builtin--convert-pkg_web-to-use-cjs-instead-of-js

Conversation

@KrauseStefan

Copy link
Copy Markdown
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Currently the pkg_web rule fails if the package.json type field is set to module in contrast to using the default common js modules.

Issue Number: N/A

What is the new behavior?

The build now works in either situation by fixing the files to be common js no matter the configuration in package.json

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@google-cla

google-cla Bot commented Jul 6, 2022

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@KrauseStefan
KrauseStefan force-pushed the fix-builtin--convert-pkg_web-to-use-cjs-instead-of-js branch 2 times, most recently from f68e8ea to 30b7719 Compare July 6, 2022 07:27
@KrauseStefan
KrauseStefan force-pushed the fix-builtin--convert-pkg_web-to-use-cjs-instead-of-js branch from 30b7719 to cda3300 Compare July 6, 2022 07:42
@KrauseStefan
KrauseStefan marked this pull request as ready for review July 6, 2022 09:23
@KrauseStefan

Copy link
Copy Markdown
Contributor Author

I am missing tests for this, and I am very much in doubt how I would go about doing that.
Any advice would be great :)

@github-actions

github-actions Bot commented Jan 6, 2023

Copy link
Copy Markdown
Contributor

This Pull Request has been automatically marked as stale because it has not had any activity for 6 months. It will be closed if no further activity occurs in 30 days. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

@github-actions github-actions Bot added the Can Close? We will close this in 30 days if there is no further activity label Jan 6, 2023
@KrauseStefan

Copy link
Copy Markdown
Contributor Author

Any chance this can be rewied and eventually approved?

@github-actions github-actions Bot removed the Can Close? We will close this in 30 days if there is no further activity label Feb 3, 2023
@gregmagolan

gregmagolan commented Feb 3, 2023

Copy link
Copy Markdown
Collaborator

Any chance this can be rewied and eventually approved?

Looks simple enough. I'll merge it in.

rules_nodejs is unmaintained at this point (since Q4 last year) and no new maintainers have stepped up. You should consider migrating to rules_js. Check out the migration guide. You can also get community support for rules_js on the #javascript channel on Bazel slack.

@gregmagolan
gregmagolan merged commit d36a73a into bazel-contrib:stable Feb 3, 2023
@KrauseStefan

Copy link
Copy Markdown
Contributor Author

Thank you!
I have been looking a lot at rules_js and I do see great merits primarily in relation to package management. However I think it is best to wait with migration till angular/components migrate if they do.
Although I am puzzled why they haven't migrated or taken over maintenance yet...

@gregmagolan

Copy link
Copy Markdown
Collaborator

The Angular team is currently looking into migrating their OSS build to rules_js as well tho they haven't pulled the trigger yet on starting it. We'd need an answer to their ng_module but I believe that we can take the same worker approach we took in ts_project (in rules_js) to make a new ng_project rule that would have the same performance that ng_module does.

@gregmagolan

Copy link
Copy Markdown
Collaborator

You can already use ts_project with ngc without worker mode. Example found here, https://github.com/aspect-build/bazel-examples/tree/main/angular

@KrauseStefan
KrauseStefan deleted the fix-builtin--convert-pkg_web-to-use-cjs-instead-of-js branch February 28, 2023 05:49
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.

2 participants