Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Package now consists of ES2015 and commonjs #1576

Merged
merged 9 commits into from
Jan 25, 2018

Conversation

rosskevin
Copy link
Contributor

@rosskevin rosskevin commented Jan 25, 2018

As discussed in apollo-maintainers target package creation to support main as commonjs and module as ES2015.

/cc @jaydenseric

Closes #1575

@rosskevin rosskevin changed the title Package mjs cjs Package now consists of ES2015 and commonjs Jan 25, 2018
Copy link
Contributor

@jbaxleyiii jbaxleyiii left a comment

Choose a reason for hiding this comment

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

@rosskevin a few questions here

@@ -90,28 +104,26 @@
"babel-core": "6.26.0",
"babel-jest": "22.1.0",
"babel-preset-env": "1.6.1",
"bundlesize": "0.15.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

@rosskevin why are we removing bundlesize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We aren't creating a UMD bundle anymore, not sure if we can size a non-compressed folder of files. I'll check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added back to test rollup-produced CJS file

"rimraf": "2.6.2",
"rollup": "0.54.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

do we not need this anymore? Does typescript handle all of the bundling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tsc does 100% of the transpilation - no more bundles (discussed with @jaydenseric in the thread)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rollup added back for bundlesize only

@@ -5,37 +5,40 @@
"module": "src/index.ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this work, does main and module get added to the package.json that actually gets published?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ugh, that's no good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wait, no, this is ok - it's there for codesandbox. Pay no attention to the /package.json, just to lib/package.json

@jbaxleyiii
Copy link
Contributor

L:ooks like the bundlesize calculation is wrong but I'll fix that on master

@jbaxleyiii jbaxleyiii merged commit 18939c2 into apollographql:master Jan 25, 2018
@rosskevin
Copy link
Contributor Author

I confirmed this PR locally with a package build using ts-loader

@rosskevin rosskevin deleted the package-mjs-cjs branch January 25, 2018 13:14
@jaydenseric
Copy link
Contributor

I'm confused, I'm looking at what is published in react-apollo@^2.1.0-beta.0 and it is nothing like discussed in Slack:

screen shot 2018-01-29 at 3 51 04 pm

It should be:

{
  "main": "index",
  "module": "index.mjs"
}

We discussed minification and bundling, and agreed that it would only happen to test bundle size but no UMD or AMD would be distributed. Currently the main points to a minified react-apollo-umd.js!

Also, many of the .mjs files are invalid as they have require in them, see #1589.

I'm trying to work out what has been going on looking at the release descriptions, and they don't make sense. v2.1.0-alpha.1 and v2.1.0-alpha.2 have the exact same bullet list of changes.

No release mentions .mjs anywhere, which it should because it's a pretty big deal.

@rosskevin
Copy link
Contributor Author

@jaydenseric @jbaxleyiii changed the main in #1578.

You should look at this changeset, it is exactly as discussed.

I'll place the rest of my comments in #1589.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants