Non Relative Imports #32

enriched · 2018-10-24T19:04:05Z

This basically enables creating the node_modules directory in a more specific way. Instead of symlinking a node_modules folder directly it constructs a node_modules folder with all of the module folders symlinked. That allows for mixing in other modules that can be resolved using the typical node module resolution strategy.

I also added in the concept of a JsContext that wraps a rule context. I attempted to do this in a similar way to rules_go. The JsContext exposes a function js.library_info(js, attr) which exposes some common functionality for extracting common javascript attributes.

The beginnings of common sharable actions are in the internal/common/actions directory. Currently there is an action for running javascript with node, and an action for creating a source directory.

This change is

enriched · 2018-10-24T19:09:48Z

@fwouts I think that this is ready for some review now. There is a lot of work around having some more shared generic js Providers in internal/common/context.bzl. I thought that it would make more sense to have this generic JsLibraryInfo provider that rule attributes get sorted into. Then some additional providers that could be extracted from the JsLibraryInfo + (some additional information). The JsModuleInfo has a lot of overlap with the JsLibraryInfo but is intended to be able to define modules without being 1:1 with a rule.

examples/node-typescript-app/tsconfig.json

fwouts · 2018-10-24T20:24:36Z

internal/common/actions/create_source_dir/create_source_dir.js

+ * mr: Recursive module (add all the modules to node_modules)
+ * g: Generate (run the passed in script to generate source files
+ *
+ * eg. node create_source_dir.js s:file/to/symlink.


Add --into and --from to the example?

fwouts · 2018-10-24T20:28:50Z

internal/common/actions/create_source_dir/create_source_dir.js

+ * symlink=true (default)
+ *
+ *
+ * The files to be populated have flags set on them for the appropriate actions:


This is awesome.

Thanks! I was trying to make it fairly generic. Would love to hear what you think about the syntax of flags:some/path:some/other/path:or_a_parameter I was originally thinking of using URLs with params. But it didn't seem like it would be well suited for specifying associations between multiple paths, which is what this thing needs to do well.

I think that should work pretty well. We should always be able to rethink this if need be at a later point.

An alternative would be to use something like a Base64-encoded JSON object or protocol buffer message, removing any issues about special characters and giving us full flexibility in the structure of the object. But that's probably overkill at this point!

internal/common/actions/create_source_dir/create_source_dir.js

fwouts

🚀 🎉 🎆 Amazing.

There are a few slightly unrelated changes in here (Prettier, upgrading rules_nodejs, reformatting with Buildifier, etc).

Do you mind if we split each of them into their own PR? I don't mind doing this to save you the extra work.

fwouts · 2018-10-24T20:29:24Z

internal/common/actions/create_source_dir/create_source_dir.js

+ * r: Recurse
+ *   rs:./some/dir/
+ * m: Module (put it in node_modules)
+ * mr: Recursive module (add all the modules to node_modules)


What does "all the modules" mean here? This module and all its dependencies?

This should probably be mrs:./some/node_modules basically it is able to take in a node modules directory and do a 1 deep symlinking of it. It will go an additional layer down for directories that begin with "@".

OK, do you mind updating the code comment with this info?

Pushed an update to the documentation for this.

fwouts · 2018-10-24T20:29:37Z

internal/common/actions/create_source_dir/create_source_dir.js

+  async args => {
+    const { current_target, workspace_name, package_path, from, into } = args;
+    const sources = ensureArray(args._);
+    // console.log(sources);


Remove this? (same below)

Alternatively, should we have a global DEBUG constant we can switch on/off?

fwouts · 2018-10-24T20:33:57Z

internal/common/actions/run_js/action.bzl

@@ -0,0 +1,43 @@
+load("//internal/npm_packages:rule.bzl", "NpmPackagesInfo")
+
+def run_js(


Thanks for this. Rules that use it look a lot better.

I agree, I think that I have it about where I want it. I was trying to abstract over some of the Bazel specific stuff with the combination of this and the BazelAction js function. Currently the primary thing that it handles is parsing a param file if bazel decides to generate one. But would be nice in the future to have it expose the ability to be run as a bazel worker. But I haven't looked too deeply into that yet.

internal/common/context.bzl

internal/ts_library/rule.bzl

fwouts · 2018-10-24T20:39:02Z

internal/ts_library/rule.bzl

+    outputs = {
+        "compilation_src_dir": "%{name}_compilation_src",
+        "compiled_dir": "%{name}_compiled",
+        # "transpilation_src_dir": "%{name}_transpilation_src",


Is this no longer needed?

I don't think so, but I know that you added it in the first place because using TypeScripts fast transpilation created a shorter critical path. Not sure if there are other rules using these outputs and looks like I might find out once I get to fixing the build for this PR.

enriched · 2018-10-24T21:04:50Z

@fwouts I definitely don't mind putting the prettier, buildifier, and updated rules_nodejs in separate PRs. I probably don't have time to do so todya, so if you can get to it sooner much appriciated 🙇

fwouts · 2018-10-24T21:13:00Z

@fwouts I definitely don't mind putting the prettier, buildifier, and updated rules_nodejs in separate PRs. I probably don't have time to do so todya, so if you can get to it sooner much appriciated 🙇

On it :)

Still might be a good idea to implement this using URIs instead though.

enriched

Removed some of the easy to get rid of cruft

internal/common/context.bzl

olegsmetanin · 2018-10-27T10:07:04Z

BTW, there is module_name attribute in https://github.com/bazelbuild/rules_typescript for building libraries, so you can use it in absolute imports.

https://github.com/bazelbuild/rules_typescript/blob/10ff84f74bfa175ea86d49e090dcc38c5ca555b6/examples/some_library/BUILD.bazel#L23

https://github.com/bazelbuild/rules_typescript/blob/0a2d6be0a10bfc759d9255f70af5867f7aebf661/examples/bar.ts

fwouts · 2018-11-21T09:05:47Z

Hey @enriched, what are the next steps to unblock this PR?

enriched · 2018-11-29T19:56:31Z

Sorry I have been so quiet on this, ran out of time and needed to more forward but I'm hoping to wrap this one up soon.

@olegsmetanin my current understanding of the module_name attribute is that it is discovered by the module_mappings_runtime_aspect the rules_nodej implements the aspect as well but neither makes the aspect public. We could reimplement the aspect in these rules, but it seems like a strange pattern for everyone to implement an aspect that looks for module_name or module_root and assumes that is a non relative module. A better pattern would be to have a shared javascript provider as proposed in bazel-contrib/rules_nodejs#57. This PR creates some common providers that can at least be used internally to bazel-javascript.

@fwouts To unblock this I think all of the rules need to use js_context and used the shared providers defined in common:

I would also appreciate any comments you have around the architecture of these. They are modeled after the go providers:

My current thinking for what these represent for these rules:

JsLibraryInfo

Metadata that is extracted from the common js rule attributes by the js_library_info helper.

JsSourceInfo

Not sure if I actually need this one. But it is the information to create a transpilation source with the common create_source_dir action. It is intended to have information about file toplolgies and scripts that generate files in the transpilation source.

JsModuleInfo

Represents a javascript module, with the information about its transitive dependencies, the non-relative import name, and the path to the imported file/folder.

fwouts · 2018-12-08T12:25:20Z

Apologies @enriched, I'm just coming back from two consecutive trips. I'll try and take a look in detail tomorrow with a fresh mind.

fwouts

This is quite large so I'm struggling to wrap my head around it all :) But the overall structure of the providers looks reasonable to me. We can remove JsSourceInfo later if need be, but it seems like a useful abstraction for now.

In terms of making the other rules start using js_context, would you like me to try and migrate a couple to get my hands dirty?

fwouts · 2018-12-09T20:24:52Z

internal/common/actions/create_source_dir/action.bzl

+    script_args.add("--into", create_dir)
+    script_args.add("--from", js.package_path)
+
+    for gen_script in gen_scripts:


This could use a few comments, what is gen_scripts?

fwouts · 2018-12-09T20:28:38Z

internal/common/context.bzl

+        all_dep_module_targets = all_dep_module_targets,
+    )
+
+def _library_to_source_info(js, library, gen_scripts = None):


Would you mind adding some comments about gen_scripts here?

Part of this is that I am still a little unsure about what the gen_scripts signature should look like. Currently the example is internal/ts_library/gsconfig.gen.js. The file exports a function as module.exports eg(with typescript types for some added clarity):

module.exports = async (ctx: {package: string, into: string, inputs: string[] }): Promise<{path: string, body: string}[]> => {}

OK—that looks like a good start, we can always adjust as we go later.

fwouts · 2018-12-09T20:34:27Z

internal/common/context.bzl

+
+    Args:
+    js: JsContext object
+    script_file: File object for the script to be run


I assume this is currently missing from the args because you haven't gotten around to that part?

This was removed and I forgot to remove it from the args. Now the script is not specified when creating an args object. The run_js action needs the script anyway because it needs to pass an array with the script followed by the args object.

fwouts · 2018-12-09T20:39:52Z

internal/common/context.bzl

+    "js",
+    # The source js files
+    "srcs",
+    # List of scripts to output


This could also be a bit more specific.

internal/common/context.bzl

…ncies

This makes it obvious that the file targets are coming from teh srcs attribute.

fwouts · 2019-01-09T21:20:45Z

Happy new year @enriched! 🎉

I'd like to make sure we get this merged within the next month or two. Please let me know what I can do to help :)

fwouts · 2019-04-24T21:23:01Z

@enriched Are you planning to continue work on this PR, or should we pick it up where you left off?

Rich Adams and others added 18 commits October 11, 2018 06:18

Adding more granular node_modules creation

b1da932

Updating rules_nodejs dependency

df6ff06

Adding prettier configuration

7d664df

Using non-relative imports in node app example

70313be

Removing some cruft

96d1baa

Removing print statements

19b70f9

Adding generation of JsModuleInfo

ea9dc58

Removing unused script

5d1111e

Adding module collection

aceaae5

Format with buildifier

4ac8ccc

Adding module target depsets for action inputs

d1cb028

Use module name from params

4c3a904

Removing verbose logging

f6f4c31

Adding the original js ctx to all providers

b79aa66

Resolving modules all the way to their package directory

75f6773

Commit formatted example files

76fce50

Removing verbose log

9976189

Fix example

63afafe

Fixing exports_files

1635857

fwouts reviewed Oct 24, 2018

View reviewed changes

enriched added 2 commits October 24, 2018 13:53

Fixing comments per PR review

e4319ff

Fixing documentation per PR comments

083104f

enriched added 4 commits October 24, 2018 14:30

Removing invalid documentation

d37eede

Still might be a good idea to implement this using URIs instead though.

Merge remote-tracking branch 'upstream/master' into non-relative-imports

8ec00d4

Removign extra comments per PR review

4772197

Removing unused JsModuleMap

8c523eb

Removing cruft per PR comments

bd639f6

enriched commented Oct 24, 2018

View reviewed changes

internal/common/context.bzl Outdated Show resolved Hide resolved

enriched added 6 commits October 24, 2018 16:18

Fixing prettier formatting

91674e1

Buildifier formatting

c75c9d9

Removing unused _compiled_extension function

69c8d71

Fixing documentation of nod_modules folder symlinking

43e67be

Adding statement about org folders

5c2910d

Removing script path from creation of script_args

92e49e3

Merge remote-tracking branch 'upstream/master' into non-relative-imports

5916658

fwouts reviewed Dec 9, 2018

View reviewed changes

enriched added 12 commits December 9, 2018 15:18

Adding filegroup to add all of the common js files to a rules depende…

489277e

…ncies

using src_files and all_src_files to make clearer

4df8c1c

This makes it obvious that the file targets are coming from teh srcs attribute.

Removing unneeded script from the args

7dfff1d

Adding in index file for the common scripts

7c29fbf

Removing unneeded script

57e2761

Moving js_binary to a BazelAction

71e4ce3

Fix missing colon

b411301

Fix references to JsLibraryInfo and JsModuleInfo

c8aa576

Move js_library to new shared context

21e2ebf

Fixing incorrect tag on NODE_MODULE_MARKER

7d336aa

Just a little better documentation on generator scripts

f165b7d

Updating documentation strings

655b9b2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Non Relative Imports #32

Non Relative Imports #32

enriched commented Oct 24, 2018 •

edited by fwouts

Loading

enriched commented Oct 24, 2018

fwouts Oct 24, 2018

fwouts Oct 24, 2018

enriched Oct 25, 2018

fwouts Oct 25, 2018

fwouts left a comment •

edited

Loading

fwouts Oct 24, 2018

enriched Oct 24, 2018

fwouts Oct 25, 2018

enriched Oct 25, 2018

fwouts Oct 24, 2018

fwouts Oct 24, 2018

enriched Oct 25, 2018

fwouts Oct 24, 2018

enriched Oct 24, 2018

enriched commented Oct 24, 2018

fwouts commented Oct 24, 2018

enriched left a comment

olegsmetanin commented Oct 27, 2018

fwouts commented Nov 21, 2018

enriched commented Nov 29, 2018

fwouts commented Dec 8, 2018

fwouts left a comment

fwouts Dec 9, 2018

fwouts Dec 9, 2018

enriched Dec 10, 2018

fwouts Dec 10, 2018

fwouts Dec 9, 2018

enriched Dec 10, 2018

fwouts Dec 9, 2018

fwouts commented Jan 9, 2019

fwouts commented Apr 24, 2019

		@@ -0,0 +1,43 @@
		load("//internal/npm_packages:rule.bzl", "NpmPackagesInfo")

		def run_js(

Non Relative Imports #32

Are you sure you want to change the base?

Non Relative Imports #32

Conversation

enriched commented Oct 24, 2018 • edited by fwouts Loading

enriched commented Oct 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwouts left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enriched commented Oct 24, 2018

fwouts commented Oct 24, 2018

enriched left a comment

Choose a reason for hiding this comment

olegsmetanin commented Oct 27, 2018

fwouts commented Nov 21, 2018

enriched commented Nov 29, 2018

JsLibraryInfo

JsSourceInfo

JsModuleInfo

fwouts commented Dec 8, 2018

fwouts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fwouts commented Jan 9, 2019

fwouts commented Apr 24, 2019

enriched commented Oct 24, 2018 •

edited by fwouts

Loading

fwouts left a comment •

edited

Loading