Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(@aws-amplify/ui-react): Export CJS #5324

Merged
merged 9 commits into from
Apr 8, 2020
Merged

fix(@aws-amplify/ui-react): Export CJS #5324

merged 9 commits into from
Apr 8, 2020

Conversation

ericclemmons
Copy link
Contributor

@ericclemmons ericclemmons commented Apr 8, 2020 •
edited
Loading

Issue #, if available: #5322

Stencil's demo project didn't transpile CJS. This PR re-uses much of aws-amplify-react.

node_modules/@aws-amplify/ui-react/dist/index.js:1
export * from './components'
^^^^^^

SyntaxError: Unexpected token 'export'

See commits for history.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ericclemmons ericclemmons added the bug Something isn't working label Apr 8, 2020
@ericclemmons ericclemmons self-assigned this Apr 8, 2020
ericclemmons
ericclemmons commented Apr 8, 2020
View reviewed changes
scripts/build.js
// tsconfig for ESM generating
let compilerOptions = {
esModuleInterop: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this, import React from "react" results in:

Cannot read property 'Component' of undefined

See: kulshekhar/ts-jest#146 (comment)

@ericclemmons ericclemmons linked an issue Apr 8, 2020 that may be closed by this pull request
@aws-amplify/ui-react doesn't transpile ES6 to ES5 #5322
Closed
@codecov
Copy link

codecov bot commented Apr 8, 2020 •
edited
Loading

Codecov Report

Merging #5324 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5324   +/-   ##
=======================================
  Coverage   72.90%   72.90%           
=======================================
  Files         195      195           
  Lines       11466    11466           
  Branches     2157     2157           
=======================================
  Hits         8359     8359           
  Misses       2956     2956           
  Partials      151      151

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 840fcce...417330a. Read the comment docs.

@ericclemmons ericclemmons mentioned this pull request Apr 8, 2020
Closed
jordanranz
jordanranz approved these changes Apr 8, 2020
View reviewed changes
Copy link
Contributor

@jordanranz jordanranz left a comment

Choose a reason for hiding this comment

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

LGTM

sammartinez
sammartinez reviewed Apr 8, 2020
View reviewed changes
scripts/build.js
@@ -148,9 +148,14 @@ function reportWatchStatusChanged(diagnostic, newLine, options, errorCount) {
}

async function buildES5(typeScriptCompiler) {
const jsx = packageInfo.name === 'aws-amplify-react' ? 'react' : undefined;
const jsx = ['@aws-amplify/ui-react', 'aws-amplify-react'].includes(
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 have includes in our library? I believe we are using find but just want to make sure there are no build errors with using this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Array.prototype.includes has been in since Node 6, and it works locally & in CI.

If there's another aspect I'm unaware of, I'm happy to swap it to .find.

Copy link
Contributor

Choose a reason for hiding this comment

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

.find is what we use now, I believe it was something on the polyfill piece of it. @manueliglesias Do you have any clarity on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

AS per our offline, I am fine. Approving

@ericclemmons ericclemmons requested a review from sammartinez April 8, 2020 17:48
sammartinez
sammartinez approved these changes Apr 8, 2020
View reviewed changes
Copy link
Contributor

@sammartinez sammartinez left a comment

Choose a reason for hiding this comment

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

LGTM 🌮

@ericclemmons ericclemmons merged commit 767025f into aws-amplify:master Apr 8, 2020
@ericclemmons ericclemmons deleted the 5282-exports-es5 branch April 8, 2020 20:10
@ThomasCarca ThomasCarca mentioned this pull request Apr 21, 2020
Closed
@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sammartinez sammartinez sammartinez approved these changes

@jordanranz jordanranz jordanranz approved these changes

@Amplifiyer Amplifiyer Awaiting requested review from Amplifiyer

@ashika01 ashika01 Awaiting requested review from ashika01

Assignees

@ericclemmons ericclemmons

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

@aws-amplify/ui-react doesn't transpile ES6 to ES5
3 participants
@ericclemmons @jordanranz @sammartinez