Skip to content

Conversation

@mocca102
Copy link
Contributor

No description provided.

@mocca102 mocca102 requested a review from a team July 17, 2023 01:48
Copy link
Contributor

@esezen esezen left a comment

Choose a reason for hiding this comment

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

@mocca102 This is looking good. I left some comments, let me know what do you think about them. We can adjust or ignore if you don't like them

README.md Outdated

### Known Issues

**Javascript version**
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do "Older Javascript versions"?

README.md Outdated

**Javascript version**

The default exported javascript syntax is ESNext, the latest supported syntax of JavaScript.
Copy link
Contributor

@esezen esezen Jul 21, 2023

Choose a reason for hiding this comment

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

What do you think about this?

This library exports two different versions

  • CommonJS (cjs)
  • ECMAScript Modules (mjs)

If you are using an import statement, mjs version will be imported.
If you are using a require statement, cjs version will be imported.

Since mjs version is compiled to ESNext, there could be features that are not supported by older Node versions (ex. optional chaining). If you are receiving an error similar to the following error, please import the cjs version which is compiled to ES2015.

import CioQuiz from '@constructor-io/constructorio-ui-quizzes/commonJs'

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are using an import statement, mjs version will be imported.
If you are using a require statement, cjs version will be imported.

Settled on Slack

@esezen esezen requested a review from jjl014 July 24, 2023 14:18
Copy link
Contributor

@esezen esezen left a comment

Choose a reason for hiding this comment

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

@mocca102 thanks for making the changes. I still think it's important to mention the differences between import and require but I might be wrong. Do you mind bringing this to standup tomorrow so we can discuss with the team?

package.json Outdated
"import": "./lib/mjs/index.js",
"require": "./lib/cjs/index.js"
},
"./commonJs": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I could swear I left a comment here but it somehow disappeared.

Do you have a preference cjs vs commonJs? If not, can we do cjs?

@mocca102 mocca102 requested a review from esezen August 1, 2023 14:54
Copy link
Contributor

@esezen esezen left a comment

Choose a reason for hiding this comment

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

LGTM!

@esezen esezen merged commit c7ce11a into main Aug 1, 2023
@esezen esezen deleted the csl-2584-os-ui-library-quizzes-update-readme-and-cjs-exports branch August 1, 2023 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants