Skip to content

react-packager: Add ES6 import statement support to DependencyGraph. - #386

Closed
pilwon wants to merge 12 commits into
react:masterfrom
pilwon:packager-es6-import-patch
Closed

react-packager: Add ES6 import statement support to DependencyGraph.#386
pilwon wants to merge 12 commits into
react:masterfrom
pilwon:packager-es6-import-patch

Conversation

@pilwon

Copy link
Copy Markdown
Contributor

This PR teaches packager's DependencyGraph how to extract dependencies written with ES6 import statements.

It fixes the issue where you are not able to write your app with ES6 import statements when your custom transformer (replacing the default JSTransform, for example, babel) already supports the ES6 import syntax.

It will also be useful for JSTransform later on once it implements import feature too.

@pilwonpilwon mentioned this pull request Mar 28, 2015
@jgable

Copy link
Copy Markdown
Contributor

Does this just work for import blah from 'blah';? Would it work for all the proper import syntaxes?

@pilwon

Copy link
Copy Markdown
ContributorAuthor

@jgable Yes, it supports all valid ES6 import syntaxes documented in the ES6 Language Specification.

You can see the result from a test ran against a comprehensive list of valid syntaxes in this gist.

@amasad

Copy link
Copy Markdown
Contributor

We also need to make the change here https://github.com/pilwon/react-native/blob/packager-es6-import-patch/packager/react-packager/src/DependencyResolver/haste/index.js#L28
It'd be great if we can consolidate them.

@pilwon

Copy link
Copy Markdown
ContributorAuthor

@amasad Sure I can add that to PR. I have a question:

Why does DependencyResolver use /require\(['"]([\.\/0-9A-Z_$\-]*)['"]\)/gi while DependencyGraph uses /\brequire\s*\(\s*[\'"]([^"\']+)["\']\s*\)/g? (they look quite different)

  • Isn't the case-insensitive flag (/i) incorrectly applied? For example, there could be a line that calls a custom function defined as function ReQuire(str) {...}. That regex will match ReQuire(...) and replace the string param that has nothing to do with the module name passed to require().
  • Is it okay \b is not present there? Wouldn't it match any function with suffix require? (ex: myrequire(...))

@pilwonpilwon mentioned this pull request Mar 31, 2015
@pilwon

Copy link
Copy Markdown
ContributorAuthor

@amasad PR #368 addressed above questions. I merged the latest code into this PR branch. Fixes #368 (comment) & test passes.

@amasad

Copy link
Copy Markdown
Contributor

Thanks, will take a look shortly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you add test where there is no space between the comma and the default?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and a test where there are no spaces between the named imports and the {}

@amasad

Copy link
Copy Markdown
Contributor

Looks good, let's just add a couple more tests

@pilwon

Copy link
Copy Markdown
ContributorAuthor

@amasad I updated test cases so most of them are in a more widely used spacing styles:

importDefault,{Foo,BarasBaz,QuxasNorf}from'x';

I still kept some different spacing style cases such as import 'x' ;, import {} from 'x';, import {Foo as Bar} from 'x';, and import Default , { } from 'x';.

Also added an incorrect statement import'x'; that is expected to be ignored by the replace regex.

@amasad

Copy link
Copy Markdown
Contributor

Great, thanks for being very response, @pilwon!

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 7, 2015
@vjeuxvjeux closed this in 3a714c3Apr 13, 2015
vjeux pushed a commit to vjeux/react-native that referenced this pull request Apr 14, 2015
Summary:
This PR teaches packager's `DependencyGraph` how to extract dependencies written with ES6 `import` statements.
It fixes the issue where you are not able to write your app with ES6 `import` statements when your custom transformer (replacing the default [JSTransform](https://github.com/facebook/jstransform), for example, [babel](http://babeljs.io/)) already supports the ES6 `import` syntax.
It will also be useful for [JSTransform](https://github.com/facebook/jstransform) later on once it implements `import` feature too.
Closesreact#386
Github Author: Pilwon Huh <pilwon@gmail.com>
Test Plan: runJestTests.sh
vjeux pushed a commit to vjeux/react-native that referenced this pull request Apr 15, 2015
Summary:
This PR teaches packager's `DependencyGraph` how to extract dependencies written with ES6 `import` statements.
It fixes the issue where you are not able to write your app with ES6 `import` statements when your custom transformer (replacing the default [JSTransform](https://github.com/facebook/jstransform), for example, [babel](http://babeljs.io/)) already supports the ES6 `import` syntax.
It will also be useful for [JSTransform](https://github.com/facebook/jstransform) later on once it implements `import` feature too.
Closesreact#386
Github Author: Pilwon Huh <pilwon@gmail.com>
Test Plan: runJestTests.sh
cpojer pushed a commit to react/metro that referenced this pull request Jan 26, 2017
Summary:
This PR teaches packager's `DependencyGraph` how to extract dependencies written with ES6 `import` statements.
It fixes the issue where you are not able to write your app with ES6 `import` statements when your custom transformer (replacing the default [JSTransform](https://github.com/facebook/jstransform), for example, [babel](http://babeljs.io/)) already supports the ES6 `import` syntax.
It will also be useful for [JSTransform](https://github.com/facebook/jstransform) later on once it implements `import` feature too.
Closesreact/react-native#386
Github Author: Pilwon Huh <pilwon@gmail.com>
Test Plan: runJestTests.sh
jfrolich pushed a commit to jfrolich/react-native that referenced this pull request Apr 22, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@pilwon@jgable@amasad@facebook-github-bot