Skip to content

Fix import statement with "subpackages" in converted ESModule #69

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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ztl8702
Copy link

@ztl8702 ztl8702 commented Jan 22, 2020

Fixes #64

Now the tricky thing is how to test it 🤔 : the test case I added in #67 would fail (error because it runs on Node.js instead of the browser. I am trying to figure out how to run rollup_test.spec.js in Karma.

@ztl8702 ztl8702 changed the title Fixes #64 Fixes import statement in converted ESModule Jan 22, 2020
@ztl8702 ztl8702 changed the title Fixes import statement in converted ESModule Fix import statement with "subpackages" in converted ESModule Jan 22, 2020
Copy link
Owner

@Dig-Doug Dig-Doug left a comment

Choose a reason for hiding this comment

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

Thanks, this looks like a good fix. I cloned your changes and managed to get it to run. I've added comments where I made changes.

// To: import {bar as foo} from "@improbable-eng/grpc-web";
return contents.replace(/var ([\w\d_]+) = require\((['"][\w\d@/_-]+['"])\)\.([\w\d_]+);/g,
(_, varName, moduleIdentifier, propertyName) =>
propertyName == varName ? `import { ${varName} } from ${moduleIdentifier}`
Copy link
Owner

Choose a reason for hiding this comment

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

Is this check needed? It makes the code look cleaner, but since this code is generated anyways I don't think it really matters. I think you could just do: 'import {$3 as $1} from $2;

test/BUILD.bazel Outdated

ts_library(
name = "rollup_test_spec",
testonly = 1,
Copy link
Owner

Choose a reason for hiding this comment

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

Optional: You can use True instead of 1

test/BUILD.bazel Outdated
],
)

ts_library(
Copy link
Owner

Choose a reason for hiding this comment

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

I was a little confused why you duplicated the rollup test until I saw your comment: "because it runs on Node.js instead of the browser".

Can you change the names of both tests to indicate what's being tested? E.g. rollup_in_node_test and rollup_in_browser_test.

test/BUILD.bazel Outdated
],
browsers = [
"@io_bazel_rules_webtesting//browsers:chromium-local",
],
Copy link
Owner

Choose a reason for hiding this comment

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

You'll need to add the files:

    # Files are served under /base/<WORKSPACE_NAME>/<path>
    static_files = [
        "@npm//:node_modules/@improbable-eng/grpc-web/dist/grpc-web-client.umd.js",
        "@npm//:node_modules/browser-headers/dist/browser-headers.umd.js",
        ":test_es6_bundling",
    ],

test/BUILD.bazel Outdated
@@ -106,13 +106,13 @@ rollup_bundle(
entry_points = {
":test_bundling.ts": "index",
},
format = "cjs",
Copy link
Owner

Choose a reason for hiding this comment

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

Make this umd so it can be loaded in both node and the browser.

You'll also need to add to the rollup config:

  output: {
    name: "test_es6_bundling",
  },

@@ -0,0 +1,49 @@
declare function require(module: string): any;
Copy link
Owner

Choose a reason for hiding this comment

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

You can do:

import * as bundle from 'test_es6_bundling';

You'll also need a declaration file to get this to compile. I couldn't figure out how to export one, so I just added:

// test/test_es6_bundling.d.ts
declare module "test_es6_bundling" {
  var a: any;
  export = a;
}

And added it to the srcs of the test.

test/BUILD.bazel Outdated
tags = ["native"],
deps = [
":rollup_test_spec",
":test_es6_bundling",
Copy link
Owner

Choose a reason for hiding this comment

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

I think this can be removed if it's in the static_files

Dig-Doug added a commit that referenced this pull request Mar 23, 2020
Dig-Doug added a commit that referenced this pull request Mar 23, 2020
- Incorporates changes from #69
@Dig-Doug Dig-Doug mentioned this pull request Mar 23, 2020
Dig-Doug added a commit that referenced this pull request Jun 30, 2020
- Incorporates changes from #69
@tsawada tsawada mentioned this pull request May 29, 2021
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.

Missing exports: unary
2 participants