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

feat(modules): build (true) esm, (interop) cjs modules; tests/readme #144

Merged
merged 32 commits into from
May 8, 2021
Merged

feat(modules): build (true) esm, (interop) cjs modules; tests/readme #144

merged 32 commits into from
May 8, 2021

Conversation

gadicc
Copy link
Owner

@gadicc gadicc commented Apr 22, 2021 •
edited
Loading

Closes #126.

Great resource: https://webreflection.medium.com/a-nodejs-dual-module-deep-dive-8f94ff56210e

Changes

Compile to both ESM and CommonJS modules, supporting both via correct fields in package.json

Type: Other New Feature

Comments/notes

  • Initial package.json and tsconfig.json changes
  • Change all requires to imports
  • Change all refs from './file' to './file.js'
  • Tests for importing from both types
  • Readme notes

@advaiyalad
Copy link
Contributor

advaiyalad commented Apr 22, 2021 •
edited by gadicc
Loading

This is just a quick rundown of what files need to be changed to use ESM.
Files that need to be changed (for require -> (maybe dynamic) import):

  • schema-tweak.js
  • env-node.ts (require() -> dynamic import())
  • cs2json.spec.ts
  • fetchDevel.js
  • moduleExec.spec.ts
  • validateAndCoerceTypes.ts
  • quoteSummary.spec.ts
  • search.spec.ts
  • setupTests.js
  • marketTimes.js
  • reporter.js
  • yahoo-finance.js
  • summary-reporter.js
  • updateJson.js

@gadicc
Copy link
Owner Author

gadicc commented Apr 24, 2021

Thanks, @PythonCreator27, you're so fast 😅

Will be a few days until I have time to work on this again but that's a great help.

@codecov
Copy link

codecov bot commented Apr 30, 2021 •
edited
Loading

Codecov Report

Merging #144 (f4872e7) into devel (eb65f14) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             devel      #144   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        20    +4     
  Lines          273       334   +61     
  Branches        77        95   +18     
=========================================
+ Hits           273       334   +61
Impacted Files Coverage Δ
src/lib/errors.ts 100.00% <ø> (ø)
src/lib/moduleExec.ts 100.00% <ø> (ø)
src/modules/autoc.ts 100.00% <ø> (ø)
src/modules/historical.ts 100.00% <ø> (ø)
src/modules/quoteSummary.ts 100.00% <ø> (ø)
src/modules/recommendationsBySymbol.ts 100.00% <ø> (ø)
src/modules/search.ts 100.00% <ø> (ø)
src/modules/trendingSymbols.ts 100.00% <ø> (ø)
src/env-node.ts 100.00% <100.00%> (ø)
src/lib/options.ts 100.00% <100.00%> (ø)
... and 22 more

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 d238274...f4872e7. Read the comment docs.

@gadicc gadicc mentioned this pull request May 2, 2021
Closed
gadicc added 23 commits May 8, 2021 08:35
@gadicc
chore(esm): initial esm work
dec552f
@gadicc
chore(esm): don't force engines while we still support CJS
d12790e
@gadicc
chore(tests): jest.config.js -> .ts, export, add dev ts-node
00113ed
@gadicc
chore(tests): update jest.config.ts to ignore /dist/ not /api/
af07200
@gadicc
chore(env): import, not require deps. add @types/node-fetch
769f2cb
@gadicc
chore(tests): ts-ignore yahooFinanceFetch partial bind
bc83053
@gadicc
chore(esm): replace require with import, add .js - breaks stuff
f3eb00e
@gadicc
chore(esm): package/jest/tsconfig bump deps, esm opts
dc4dfd7
@gadicc
chore(esm): fix manual conflict resolution for import type Optsions
ba60463
@gadicc
chore(esm): more node/jest config opts for esm, setupTests, setGlobal
cd4364c
@gadicc
chore(esm): import jest global in tests
17b0ea1
@gadicc
use relative paths in package.json for main/exports/types/browser
b024e27
@gadicc
eureka
6e6de76
@gadicc
fetchDevel: use URL paths
046975d
@gadicc
yahooFinanceFetch: this && this._env check
91f1a66
@gadicc
yahooFinanceFetch: remove console.log
735f96d
@gadicc
historical.spec.ts ts-ignore
46c55a4
@gadicc
schema-tweak.js
6ab2846
@gadicc
ci fix, update node image for es2020 for dynamic import
97a0ced
@gadicc
yahoo-finance.js bin
0dcc41f
@gadicc
package.json: add "module" field, expand "exports"
de163c0
@gadicc
build:cjs - change package.json "type"
a3ecd3b
@gadicc
yarn.lock: recreate to fix missed conflicts in rebase
fd813d9
@gadicc gadicc changed the title Esm feat(modules): build (true) esm and (interop) cjs modules, support both, tests, readme. May 8, 2021
@gadicc gadicc changed the title feat(modules): build (true) esm and (interop) cjs modules, support both, tests, readme. feat(modules): build (true) esm, (interop) cjs modules; tests/readme May 8, 2021
@gadicc gadicc merged commit 2182f6c into devel May 8, 2021
@gadicc
Copy link
Owner Author

gadicc commented May 8, 2021

Merged to devel. Hopefully we'll still have some time before next release and can see if any issues crop up. Can revert if necessary. We have sanity tests to load in both ways, clear README instructions, and I'm using this in an "interop" (babel) project that uses "es-style" CJS modules and all works as expected.

@gadicc gadicc mentioned this pull request May 8, 2021
Closed
@gadicc gadicc deleted the esm branch May 8, 2021 12:46
@advaiyalad
Copy link
Contributor

@gadicc Tests no longer work on windows or WSL. I am trying some hacks and reverting some things to make it work. I think that some parts of this PR will have to be reverted, like the tests. After reverting the .js addition and the changes to the jest config, I still failed to get tests running. node_modules/.bin/jest doesn't work on windows, see jestjs/jest#3750. Next, after fixing that, the tests fail saying that "the test environment has already been torn down", pointing at await import('./lib/fetchDevel.js').

Jest ESM support is currently poor, I think that this PR should be held off until ESM support works properly in jest, or at least until tests work on all platforms.
Also see #167.

@advaiyalad advaiyalad mentioned this pull request May 8, 2021
Merged
6 tasks
@gadicc gadicc mentioned this pull request May 9, 2021
Closed
gadicc pushed a commit that referenced this pull request Jun 1, 2021
@semantic-release-bot
chore(release): 1.11.0 [skip ci]
f6f3e60 
# [1.11.0](v1.10.4...v1.11.0) (2021-06-01)

### Bug Fixes

* **deps:** update dependency ajv to ^8.1.0 ([1769641](1769641))
* **quoteCombine:** resolve `undefined` for missing symbols ([#150](#150)) ([f8c25e3](f8c25e3))
* **testing:** specify jest.js path, not bin ([#170](#170)) ([6772c8e](6772c8e))

### Features

* **options:** accept `date` option ([#186](#186)) ([11b8a72](11b8a72))
* add (friendly) warning when used in the browser ([3c4c5a0](3c4c5a0)), closes [#153](#153)
* add insights module ([#169](#169)) ([4603232](4603232))
* **concurrency:** ability to limit max simultaneous requests ([#76](#76)) ([3424d44](3424d44))
* **modules:** build (true) esm, (interop) cjs modules; tests/readme ([#144](#144)) ([2182f6c](2182f6c))
* **setGlobalConfig:** add setGlobalConfig function ([#133](#133)) ([43ebaa4](43ebaa4))
@gadicc
Copy link
Owner Author

gadicc commented Jun 1, 2021

🎉 This PR is included in version 1.11.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gadicc gadicc added the released label Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Convert to ESM module (BREAKING). Target: end of beta cycle.
2 participants
@gadicc @advaiyalad