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.

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

Fix cadence-parser's esm package #2815

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wfalcon0x
Copy link

@wfalcon0x wfalcon0x commented Sep 24, 2023

Description

cadence-parser's build configuration is using tsc to build a dual package, however, the created esm module cannot be imported from a nodejs es module, as the "type": "module" is missing from the es module specific package.json.

This PR contains a way to solve this issue, although many other ways exist based on the choice of tooling. The solution included is an attempt to fix it with the smallest amount of change to the current toolings and configuration.


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@turbolent
Copy link
Member

Thank you! Is there an easy way to verify that this setup allows usage of the package in various environments (e.g. browser, Node.JS, etc.)?

@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.03% 🎉

Comparison is base (0639275) 79.23% compared to head (54fe090) 79.26%.
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2815      +/-   ##
==========================================
+ Coverage   79.23%   79.26%   +0.03%     
==========================================
  Files         333      333              
  Lines       78753    78769      +16     
==========================================
+ Hits        62401    62440      +39     
+ Misses      14042    14025      -17     
+ Partials     2310     2304       -6     
Flag Coverage Δ
unittests 79.26% <ø> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nialexsan
Copy link
Contributor

@wfalcon0x could you please share code where you have issues with importing this package?
with our current setup it should be possible to use the package with import and with require from nodejs apps

"main": "./dist/cjs/index.js",
"module": "./dist/esm/index.js",
"types": "./dist/types/index.d.ts",
"exports": {
".": {
"require": "./dist/cjs/index.js",
"import": "./dist/esm/index.js",
"types": "./dist/types/index.d.ts"
}
},

@wfalcon0x
Copy link
Author

@wfalcon0x could you please share code where you have issues with importing this package? with our current setup it should be possible to use the package with import and with require from nodejs apps

"main": "./dist/cjs/index.js",
"module": "./dist/esm/index.js",
"types": "./dist/types/index.d.ts",
"exports": {
".": {
"require": "./dist/cjs/index.js",
"import": "./dist/esm/index.js",
"types": "./dist/types/index.d.ts"
}
},

@nialexsan Running the following code from nodejs fails with a syntax error.

import {CadenceParser} from "@onflow/cadence-parser"

The error is:

import {CadenceParser} from "@onflow/cadence-parser"
        ^^^^^^^^^^^^^
SyntaxError: Named export 'CadenceParser' not found. The requested module '@onflow/cadence-parser' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from '@onflow/cadence-parser';
const {CadenceParser} = pkg;

    at ModuleJob._instantiate (node:internal/modules/esm/module_job:127:21)
    at async ModuleJob.run (node:internal/modules/esm/module_job:193:5)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:337:24)
    at async loadESM (node:internal/process/esm_loader:88:5)
    at async handleMainPromise (node:internal/modules/run_main:61:12)

The solution is to add a package.json into the /dist/esm folder with the following content:

{
  "type": "module"
}

@wfalcon0x
Copy link
Author

wfalcon0x commented Sep 26, 2023

Thank you! Is there an easy way to verify that this setup allows usage of the package in various environments (e.g. browser, Node.JS, etc.)?

@turbolent I tested it as follows: I first published the changes to my local npm repository, and then verified its functionality in the following scenarios:

  • In Node.js using CommonJS with the "require" syntax.
  • In Node.js using ES6 modules with the "import" syntax.
  • In a web browser, I created a React application using "create-react-app" and imported the library there.

Please note, the added dev dependency ([email protected]) requires { node: '>=18.3.0 || >=16.17.0' }

@turbolent
Copy link
Member

@nialexsan Thank you for having a look! Could you please have another look at what wfalcon0x posted?

@nialexsan nialexsan mentioned this pull request Oct 5, 2023
6 tasks
@turbolent
Copy link
Member

@wfalcon0x Thank you again for your contribution and your patience! Could you please see if #2851 improved things for you? If yes, we can maybe close this PR. Alex provides some details on how this PR might not be the way we want to go here: #2851 (comment). Hope that makes sense (I don't know much about it), but please report back if you're still running into issues. Thanks!

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

Successfully merging this pull request may close these issues.

3 participants