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(examples): add Jest example #1274

Merged
merged 6 commits into from
Oct 23, 2019

Conversation

purkhusid
Copy link
Contributor

This PR adds an example of how you might create a Jest rule

Unfortunately Jest does not handle symlinks very well so we have to add some patches so that it behaves correctly(See: jasongwartz/bazel_rules_nodejs_contrib#4 (comment))

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
  • [ x] Example:

@googlebot
Copy link

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

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@purkhusid
Copy link
Contributor Author

Note that this is not working yet since I can't get the autogenerated rule to resolve babel-loader

@alexeagle
Copy link
Collaborator

@Globegitter I'm following the threads of your work on this.

@purkhusid if the problem is really that jest-cli doesn't have a dependency declared in its package.json on something it wants to require(), this can be solved now with an attribute of yarn_install that adds missing dependency edges to Bazel's graph:

dynamic_deps = {
        "jest-cli": "@npm//@babel/preset-env",
    },

but this doesn't seem to change anything for me. Let's skip that for now and unroll the macro one step.

nodejs_test(
      entry_point = "@npm//:node_modules/jest-cli/bin/jest.js",
      install_source_map_support = False,
      data = ["@npm//jest-cli:jest-cli", "@npm//:node_modules"] + kwargs.pop("data", []),
      **kwargs
    )

note the @npm//:node_modules there which just says that the jest binary should include all of node_modules as deps.

Even then, the failure still seems like a syntax error, not a module resolution problem:

    /usr/local/google/home/alexeagle/.cache/bazel/_bazel_alexeagle/34f8c1c4c845d6e8057f92b20b01a493/sandbox/linux-sandbox/17/execroot/examples_jest/bazel-out/k8-fastbuild/bin/test.sh.runfiles/examples_jest/index2.test.js:1
    ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){import indexfile from './index.js';
                                                                                                    ^^^^^^^^^

    SyntaxError: Unexpected identifier

      at ScriptTransformer._transformAndBuildScript (../npm/node_modules/jest-runner/node_modules/@jest/transform/build/ScriptTransformer.js:537:17)

@purkhusid
Copy link
Contributor Author

@alexeagle babel.config.js was missing from deps so now I'm getting the expected error:

Cannot find module '@babel/preset-env' from '/private/var/tmp/_bazel_danielpurkhus/039f5a9ccff2ae48767831e8b1a82438/sandbox/darwin-sandbox/73/execroot/examples_jest/bazel-out/darwin-fastbuild/bin/test.sh.runfiles/examples_jest'

I believe that it's acutally babel that is complaining about not finding the package so this might be more complicated than I had imagined. So we actually have a dependency chain that looks like this:

jest-cli  -> babel-jest(Loaded in the jest.config.js) -> @babel/preset-env(loaded in the babel.config.js)

The runfiles folder structure looks like this:

- examples_jest
    - <source files>
- npm
    - node_modules
        - <modules>   

Is there any reason for that we can not symlink npm/node_modules into the root of the examples_jest workspace? That way I think most tools should work since they usually traverse the folder structure upwards until they find a package.json or a node_modules directory.

@purkhusid
Copy link
Contributor Author

@alexeagle @Globegitter If there are any ideas on how we might be able to recreate the traditional Node folder structure within the runfiles then I could take a stab at it. Bazel is still kind of new to me so I don't know all the available tools.

purkhusid and others added 2 commits October 22, 2019 14:44
This PR adds an example of how you might create a Jest rule

Unfortunately Jest does not handle symlinks very well so we have to add some patches so that it behaves correctly(See: jasongwartz/bazel_rules_nodejs_contrib#4 (comment))
@alexeagle
Copy link
Collaborator

I fixed the node_modules layout in bc848f9 - the Jest example is passing for me now.

We should take out some of the patches and put them into the new bazel_require_script.js whose job is to patch the nodejs environment before calling the entry script.

@alexeagle
Copy link
Collaborator

@purkhusid could you sign the CLA so I can merge this if green?

@purkhusid
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@purkhusid
Copy link
Contributor Author

@alexeagle You want the missing fs functions before you merge or just add them afterwards?

Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Cool, let's get this in, next step is for @soldair to delete the patches by making the bazel_require_script do the right thing

@alexeagle alexeagle merged commit f864462 into bazel-contrib:master Oct 23, 2019
@marcus-sa
Copy link

marcus-sa commented Nov 14, 2019

Doesn't work for me.
At first I thought it'd be a flaky test, but it keeps running into module resolution issues.
It can't really be wrong configuration, since it happens in this repo as well.

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.

4 participants