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 package_name & package_path to ts_library #2799

Merged
merged 1 commit into from Jul 1, 2021

Conversation

gregmagolan
Copy link
Collaborator

@gregmagolan gregmagolan commented Jun 30, 2021

Resolves #2450

BREAKING CHANGES:

module_name will no longer turn on linking for the ts_library target; instead package_name must now be specified to enable linking. package_path may also be specified to control the link location.

This PR includes a breaking change to simplify module_mappings_aspect in internal/linker/link_node_modules.bzl now that a ts_library special case is no longer needed.

NB: the alternative would be to remove linking support from ts_library entirely as a js_library or npm_link could be used instead, however, given that ts_library doesn't expose its .js output as the default output this would require users to use a filegroup with an output selector to maintain the same functionality as ts_library currently has with module_name. This PR keeps the BREAKING CHANGE to just adding a package_name attribute to make the ts_library js outputs linkable.

@google-cla google-cla bot added the cla: yes label Jun 30, 2021
@gregmagolan gregmagolan force-pushed the ts_library_package_name branch 2 times, most recently from 98bca9a to bb9fd46 Compare June 30, 2021 08:38
@gregmagolan gregmagolan changed the title feat: add package_name to ts_library feat: add package_name & package_path to ts_library Jun 30, 2021
@gregmagolan gregmagolan force-pushed the ts_library_package_name branch 2 times, most recently from 6d4f0a9 to 3201602 Compare July 1, 2021 03:55
@@ -1,8 +1,53 @@
(() => {
var __create = Object.create;
Copy link
Collaborator

Choose a reason for hiding this comment

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

whoa what happened here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know. @mattem can you take a look at this golden bundle change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The import now comes via the linker I'm guessing.
Previously esbuild was picking up the mjs file from bazel-out where it's now finding the js in node_modules.
Ideally you'd set resolveExtensions to prefer the mjs over the js:

    args = {
        "resolveExtensions": [
            ".mjs",
            ".js",
        ],
    },

Perhaps we could make that the default again (it was previously, but we removed it to remove as much customization and use vanilla esbuild as much as possible).

This would then result in:

(() => {
  // node_modules/lib/index.mjs
  var NAME = "bazel";
  // bazel-out/darwin-fastbuild/bin/packages/esbuild/test/bundle/a.mjs
  console.log(`Hello ${NAME}`);
})();
//# sourceMappingURL=bundle.js.map

From the bundle metadata from your diff:

"inputs": {
    "node_modules/lib/index.js": {
      "bytes": 864,
      "imports": []
    },
    "bazel-out/darwin-fastbuild/bin/packages/esbuild/test/bundle/a.mjs": {
      "bytes": 520,
      "imports": [
        {
          "path": "node_modules/lib/index.js",
          "kind": "import-statement"
        }
      ]
    }

From HEAD on 4.x:

"inputs": {
    "bazel-out/darwin-fastbuild/bin/packages/esbuild/test/bundle/index.mjs": {
      "bytes": 395,
      "imports": []
    },
    "bazel-out/darwin-fastbuild/bin/packages/esbuild/test/bundle/a.mjs": {
      "bytes": 520,
      "imports": [
        {
          "path": "bazel-out/darwin-fastbuild/bin/packages/esbuild/test/bundle/index.mjs",
          "kind": "import-statement"
        }
      ]
    }

In esbuild, we generate the package mappings into a tsconfig file to ensure we pick them up from bazel-out, not node_modules. We could change this though.
https://github.com/bazelbuild/rules_nodejs/blob/4.x/packages/esbuild/esbuild.bzl#L36

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

brilliant. worked as predicted. thanks!

not sure about making it default as this would only be an issue with ts_library which produces both right?

@@ -6,6 +6,7 @@ package(default_visibility = ["//:__subpackages__"])
# with a main field to show how to create a first-party npm lib with a package.json
ts_library(
name = "shorten",
package_name = "@bazel/shorten",
Copy link
Collaborator

Choose a reason for hiding this comment

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

when should we drop the view engine example? is that still used by anyone in Angular land? @alan-agius4 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good question; I kept it green in this PR but I'm not against removing it as it is tied to angular v9

@@ -6,6 +6,7 @@ package(default_visibility = ["//:__subpackages__"])
# with a main field to show how to create a first-party npm lib with a package.json
ts_library(
name = "shorten",
package_name = "@bazel/shorten",
srcs = ["index.ts"],
module_name = "@bazel/shorten",
Copy link
Collaborator

Choose a reason for hiding this comment

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

should the module_name be removed now? or is that still required as the AMD module name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can't yet at it is tied to ts_library internals; this PR makes it so that module_name no longer turns on the linker as it was causing issues downstream

mn = linkable_package_info.package_name
mr = ["__link__", linkable_package_info.path]

# Special case for ts_library module_name for legacy behavior and for AMD name work-around
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎉

BREAKING CHANGES:

module_name will no longer turn on linking for the ts_library target; instead package_name must now be specified to enable linking. package_path may also be specified to control the link location.

This PR includes a breaking change to simplify module_mappings_aspect in internal/linker/link_node_modules.bzl now that a ts_library special case is no longer needed.

NB: the alternative would be to remove linking support from ts_library entirely as a js_library or npm_link could be used instead, however, given that ts_library doesn't expose its .js output as the default output this would require users to use a filegroup with an output selector to maintain the same functionality as ts_library currently has with module_name. This PR keeps the BREAKING CHANGE to just adding a package_name attribute to make the ts_library js outputs linkable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants