Support & test import() for non-bundled code #1615

Timer · 2017-02-22T18:20:24Z

No description provided.

Timer · 2017-02-23T05:10:41Z

Well... this is weird. "Test basic import syntax" passed ... it should've failed. 🤔
Does Jest support import() or is something else in our tool chain handle this?

edit: ignore this, it does fail but not on CI -- why? see below.

Timer · 2017-02-23T05:44:34Z

Now I'm just really confused .. fails locally but not on CI? 🌀

gaearon · 2017-02-23T12:31:10Z

Maybe we could try running Jest with --no-cache on CI just in case.

Timer · 2017-02-23T14:48:04Z

I believe we already do: https://github.com/facebookincubator/create-react-app/blob/d822d8d479ef7986b394f39775aa1dc46540e7b2/tasks/e2e-kitchensink.sh#L138

gaearon · 2017-02-23T14:56:02Z

I want to see a failing CI test 😢

Timer · 2017-02-23T15:19:30Z

I wonder if our CI tests even work ...
I'm going to try to look more into this.

Timer · 2017-02-23T15:19:59Z

/cc @EnoahNetzach

EnoahNetzach · 2017-02-24T11:22:19Z

Back when I was working on CI tests, I tried to make it fail (e.g. with #1160), and my tests always failed correctly..

Also, running this branch in a docker container EnoahNetzach:create-react-app/local-testing won't fail.

Did you link babel-preset-react-app? I just tested it and without the link it was failing.

EnoahNetzach · 2017-02-24T11:28:01Z

packages/react-scripts/fixtures/kitchensink/src/features/syntax/Promises.test.js


 describe('promises', () => {
  it('renders without crashing', () => {
    const div = document.createElement('div');
-    return new Promise(resolve => {
-      ReactDOM.render(<Promises onReady={resolve} />, div);
+    return import('./Promises').then(({ default: Promises }) => {


wouldn't it be better to give import its own test case?

Timer · 2017-02-25T02:56:11Z

@gaearon I think the tests are just flaky (for an unknown reason).
Notice the fail and pass when I revert and then revert the revert.

Should I create a new PR?

gaearon · 2017-02-25T03:19:31Z

Flaky as in "sometimes fails when it shouldn't" is understandable and fine.
Flaky as in "sometimes passes when it shouldn't" is weird and we need to figure out why.

Timer · 2017-02-25T04:46:52Z

Okay, so it passes when the other commit it present. a373aec fails when c2ab8fb is missing, but a373aec starts passing when c2ab8fb is present.

So I'm not sure if this is a travis bug or a e2e bug (or just how travis works). If it was a e2e bug that means we're somehow grabbing the latest. A travis bug would mean they're grabbing the latest PR and not that specific commit.

Timer · 2017-02-25T04:49:56Z

Looks like travis does git fetch origin +refs/pull/1615/merge: and git checkout -qf FETCH_HEAD which means Travis is only capable of testing the latest given commit in a PR, not older commits.

tl;dr this should be safe to land

gaearon · 2017-02-25T04:51:00Z

Do you mean that it might display a previous commit has passed while in reality it has grabbed the most recent pushed commit (that hasn't reached the queue yet)?

Timer · 2017-02-25T04:51:41Z

That's exactly what it does. But only for PRs.
Our master branch checks out the commit like it should, e.g. git checkout -qf 3289c3202149980b0ce4326e39d5501ee88d5284 (so no need to worry there).

Basically if you push a broken commit A, and then push a working commit B before the travis job starts for commit A, commit A will pass because it grabs commit B.

gaearon · 2017-02-25T04:53:23Z

OK, that makes sense.
So what happened is:

You push commit A
Travis enqueues a job
You push commit B
Travis enqueues another job
Travis executes the first job, effectively fetching commit B, but GH integration shows the checkmark for A because that’s what the job was queued for
Travis executes the second job

Timer · 2017-02-25T04:54:06Z

That would be it. 😄

edit for people for people looking from twitter: this only affects pull requests, not branches

gaearon · 2017-02-25T04:54:45Z

OK today I learned. 😅

Timer · 2017-02-25T04:59:18Z

I noticed this when I pushed the second commit shortly after the first, because Node 4 kitchensink had started (and failed) and then the Node 6 kitchensink (queued not executing when I pushed second commit) succeeded. I then re-validated my theory by only running the first commit alone then pushing the second commit and re-running the first job, seeing it magically switch from failing to passing.

Think this is worthwhile enough to report to travis? Prob the intended functionality... Confusing never the less.

gaearon · 2017-02-25T04:59:38Z

Meh, I guess this is as designed.

gaearon · 2017-02-25T05:00:01Z

Good job figuring it out tho

Timer · 2017-02-25T05:00:09Z

Thanks! Good to land?

gaearon · 2017-02-25T05:01:46Z

Yep

mhart · 2017-02-25T14:43:40Z

Just to clarify, because the wording of "effectively fetching commit B" confused me a bit:

This is because Travis wants to use the post-merge commit – ie, when a push on a PR happens and GitHub checks if it is "ok to merge", GitHub actually performs the merge itself between the current state of the PR branch and the branch it's been opened on (eg, master).

The resulting commit (which is only available remotely, as something like +refs/pull/1615/merge – a moving target) is the one that Travis is trying to use because it's a more comprehensive check of how things will probably look if the PR is merged. It could of course just use the commit that was pushed, which wouldn't have race conditions, but that commit could be out of date w/ respect to the branch the PR has been opened on (ie, master could have a whole bunch more commits on it since the PR was opened), so wouldn't be as safe a representation of how things will look post-merge.

If GitHub offered a reference like refs/pull/1615/merge-<commit-sha> then this wouldn't be an issue (but it would also require an order of magnitude more references on GH's end).

* Test basic import syntax * Compile import() in test, only support syntax otherwise

Timer added this to the 0.10.0 milestone Feb 22, 2017

facebook-github-bot added the CLA Signed label Feb 22, 2017

Timer mentioned this pull request Feb 22, 2017

Add lint rule to disallow require.ensure and System.import #1536

Merged

Timer force-pushed the defer-require branch from d849b7c to e1a124c Compare February 22, 2017 19:27

Timer force-pushed the defer-require branch 4 times, most recently from 9c57a84 to 2ac84ea Compare February 23, 2017 06:05

EnoahNetzach reviewed Feb 24, 2017

View reviewed changes

Timer force-pushed the defer-require branch 2 times, most recently from 747263e to 40ac3c6 Compare February 25, 2017 00:51

Test basic import syntax

a373aec

Timer force-pushed the defer-require branch from b772076 to a373aec Compare February 25, 2017 03:26

Compile import() in test, only support syntax otherwise

c2ab8fb

Timer merged commit 2288ddf into facebook:master Feb 25, 2017

Timer deleted the defer-require branch February 25, 2017 05:03

Timer added the tag: internal label Feb 25, 2017

Timer mentioned this pull request Feb 27, 2017

Add support for dynamic import() #1192

Closed

5 tasks

kst404 pushed a commit to kst404/e8e-react-scripts that referenced this pull request Mar 2, 2017

Support & test import() for non-bundled code (facebook#1615)

8d29cbd

* Test basic import syntax * Compile import() in test, only support syntax otherwise

lock bot locked and limited conversation to collaborators Jan 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support & test import() for non-bundled code #1615

Support & test import() for non-bundled code #1615

Timer commented Feb 22, 2017

Timer commented Feb 23, 2017 •

edited

Loading

Timer commented Feb 23, 2017 •

edited

Loading

gaearon commented Feb 23, 2017

Timer commented Feb 23, 2017

gaearon commented Feb 23, 2017

Timer commented Feb 23, 2017 •

edited

Loading

Timer commented Feb 23, 2017

EnoahNetzach commented Feb 24, 2017 •

edited

Loading

EnoahNetzach Feb 24, 2017

Timer commented Feb 25, 2017

gaearon commented Feb 25, 2017

Timer commented Feb 25, 2017 •

edited

Loading

Timer commented Feb 25, 2017 •

edited

Loading

gaearon commented Feb 25, 2017

Timer commented Feb 25, 2017 •

edited

Loading

gaearon commented Feb 25, 2017 •

edited

Loading

Timer commented Feb 25, 2017 •

edited

Loading

gaearon commented Feb 25, 2017

Timer commented Feb 25, 2017 •

edited

Loading

gaearon commented Feb 25, 2017

gaearon commented Feb 25, 2017

Timer commented Feb 25, 2017

gaearon commented Feb 25, 2017

mhart commented Feb 25, 2017 •

edited

Loading

Support & test import() for non-bundled code #1615

Support & test import() for non-bundled code #1615

Conversation

Timer commented Feb 22, 2017

Timer commented Feb 23, 2017 • edited Loading

Timer commented Feb 23, 2017 • edited Loading

gaearon commented Feb 23, 2017

Timer commented Feb 23, 2017

gaearon commented Feb 23, 2017

Timer commented Feb 23, 2017 • edited Loading

Timer commented Feb 23, 2017

EnoahNetzach commented Feb 24, 2017 • edited Loading

EnoahNetzach Feb 24, 2017

Choose a reason for hiding this comment

Timer commented Feb 25, 2017

gaearon commented Feb 25, 2017

Timer commented Feb 25, 2017 • edited Loading

Timer commented Feb 25, 2017 • edited Loading

gaearon commented Feb 25, 2017

Timer commented Feb 25, 2017 • edited Loading

gaearon commented Feb 25, 2017 • edited Loading

Timer commented Feb 25, 2017 • edited Loading

gaearon commented Feb 25, 2017

Timer commented Feb 25, 2017 • edited Loading

gaearon commented Feb 25, 2017

gaearon commented Feb 25, 2017

Timer commented Feb 25, 2017

gaearon commented Feb 25, 2017

mhart commented Feb 25, 2017 • edited Loading

Timer commented Feb 23, 2017 •

edited

Loading

Timer commented Feb 23, 2017 •

edited

Loading

Timer commented Feb 23, 2017 •

edited

Loading

EnoahNetzach commented Feb 24, 2017 •

edited

Loading

Timer commented Feb 25, 2017 •

edited

Loading

Timer commented Feb 25, 2017 •

edited

Loading

Timer commented Feb 25, 2017 •

edited

Loading

gaearon commented Feb 25, 2017 •

edited

Loading

Timer commented Feb 25, 2017 •

edited

Loading

Timer commented Feb 25, 2017 •

edited

Loading

mhart commented Feb 25, 2017 •

edited

Loading