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

Use lodash-es for ES modules #1344

Merged
merged 5 commits into from Dec 8, 2017

Conversation

Projects
None yet
9 participants
@corydeppen
Copy link
Contributor

corydeppen commented Nov 19, 2017

Resolves #1286

@meteor-bot

This comment has been minimized.

Copy link

meteor-bot commented Nov 19, 2017

Warnings
⚠️

There are library changes, but not tests. That's OK as long as you're refactoring existing code

Generated by 🚫 dangerJS

@corydeppen

This comment has been minimized.

Copy link
Contributor Author

corydeppen commented Nov 19, 2017

Sorry for the noise - I mistakenly ran compile instead of test prior to committing so I didn't see the issue until it was too late. It appears the problem is that lodash-es ships ES modules, which aren't transformed by Jest. I'm hoping solution 3 will solve this issue.

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Nov 29, 2017

@corydeppen what is lodash-es?

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Nov 29, 2017

Ah! I see #1286 (comment)

Can you take a look at the CI failure? And add a changelog and I'd love to merge and release this!

corydeppen added some commits Nov 19, 2017

Resolve CI errors
- Update lodash types
- Add babelify to Transform node_modules/lodash-es for Browserify build
- Add changelog entry

@corydeppen corydeppen force-pushed the corydeppen:lodash-es branch from 6931bad to 7a26870 Nov 30, 2017

@corydeppen

This comment has been minimized.

Copy link
Contributor Author

corydeppen commented Nov 30, 2017

@jbaxleyiii Including functions from the specific lodash-es modules increased the size of the bundle. I didn't want to tweak the bundlesize threshold until I knew you were okay with the approach.

@bjrnt

This comment has been minimized.

Copy link

bjrnt commented Dec 5, 2017

Is there any way we could get this merged? Seems like this issue is holding up more than 20 teams from upgrading to v.2 based on the reactions to #1286.

@jbaxleyiii

This comment has been minimized.

Copy link
Member

jbaxleyiii commented Dec 8, 2017

@corydeppen I'm really concerned about including all of lodash even with the es version for people who don't have bundlers that can remove it. 😢

I wonder if there is another way?

edit I don't know why the size is increasing since it is importing the file directly. I'll take a look and see how to get this merged today!

James Baxley added some commits Dec 8, 2017

James Baxley
James Baxley

@jbaxleyiii jbaxleyiii merged commit 8085a94 into apollographql:master Dec 8, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
CLA Author has signed the Meteor CLA.
Details
@HugoGiraudel

This comment has been minimized.

Copy link

HugoGiraudel commented Dec 8, 2017

I posted this comment on a commit but I cannot seem to find it anymore so allow me to submit it again here.

This change (apparently already published in 2.0.3, despite changelog mentioning vNext) seems to cause an error in our build:

SyntaxError: Unexpected token import
    at createScript (vm.js:80:10)
    at Object.runInThisContext (vm.js:139:10)
    at Module._compile (module.js:599:28)
    at Object.Module._extensions..js (module.js:646:10)
    at Module.load (module.js:554:32)
    at tryModuleLoad (module.js:497:12)
    at Function.Module._load (module.js:489:3)
    at Module.require (module.js:579:17)
    at require (internal/module.js:11:18)
    at /goldfish/node_modules/react-apollo/react-apollo.umd.js:2:123
/goldfish/node_modules/lodash-es/pick.js:1
(function (exports, require, module, __filename, __dirname) { import basePick from './_basePick.js';

Did we miss something maybe? Everything was (is) working fine in 2.0.1.

Thank you!

@doomsower

This comment has been minimized.

Copy link
Contributor

doomsower commented Dec 8, 2017

I get same error as @HugoGiraudel here after update, but only in jest tests. The app itself builds and runs fine
I'm using create-react-app-typescript

Here is my tsconfig.json:

{
  "compilerOptions": {
    "outDir": "build/dist",
    "module": "esnext",
    "target": "es5",
    "lib": ["es7", "dom", "esnext.asynciterable"],
    "sourceMap": true,
    "jsx": "react",
    "moduleResolution": "node",
    "rootDir": "src",
    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,
    "allowSyntheticDefaultImports": false,
    "noImplicitReturns": true,
    "noImplicitThis": true,
    "noImplicitAny": true,
    "strictNullChecks": true
  },
  "exclude": [
    "__mocks__",
    "node_modules",
    "build",
    "scripts",
    "acceptance-tests",
    "webpack",
    "jest",
    "src/setupTests.ts"
  ]
}

And for tests, tsconfig.test.json:

{
  "extends": "./tsconfig.json",
  "compilerOptions": {
    "module": "commonjs"
  }
}
@doomsower

This comment has been minimized.

Copy link
Contributor

doomsower commented Dec 8, 2017

From this comment facebook/create-react-app#3001 (comment) I conclude that this problem should also affect regular create-react-app projects, not just typescript fork.

I see that in this PR @corydeppen works around this with transformIgnorePatterns, but this is impossible with create-react-app projects

KevinGrandon added a commit to KevinGrandon/react-apollo that referenced this pull request Dec 9, 2017

@lednhatkhanh

This comment has been minimized.

Copy link

lednhatkhanh commented Dec 9, 2017

Since the fix has been rolled back, now I get the lodash error again...

@danielrasmuson

This comment has been minimized.

Copy link

danielrasmuson commented Dec 10, 2017

Any known workarounds while this issue gets resolved?

@corydeppen corydeppen deleted the corydeppen:lodash-es branch Dec 27, 2017

@kandros

This comment has been minimized.

Copy link
Contributor

kandros commented Jan 5, 2018

still looking for a workaroud, any idea?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment