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(lib): Generate a module version of the library to be used as import in a devlopment source #46

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jacques-lebourgeois
Copy link
Member

@jacques-lebourgeois jacques-lebourgeois commented Dec 15, 2023

Related issues

#18

Description

Angular compilation gave a warning on CommonJS or AMD dependencies

To get code generation optimized, it is recommended that you use ECMAScript modules.

The Pull request contains:

  • the library is compiled as a module ESNext and declared as a module
  • webpack generates two bundle :
    • ods-chart.js is still used in HTML <script>
    • ods-charts-module.js is the one referenced in package.json to be used as the imported module
  • some vue and react compilation problems

Motivation & Context

Code optimization

Types of change

  • Bug fix (non-breaking which fixes an issue)
  • Refactoring (non-breaking change)

Test checklist

Please check that the following tests projects are still working:

  • docs/examples
  • test/angular-ngx-echarts
  • test/angular-tour-of-heroes
  • test/html
  • test/react
  • test/vue
  • test/examples/bar-line-chart
  • test/examples/single-line-chart

Copy link

netlify bot commented Dec 15, 2023

Deploy Preview for ods-charts ready!

Name Link
🔨 Latest commit e21dbc8
🔍 Latest deploy log https://app.netlify.com/sites/ods-charts/deploys/6740c0ddcc68d9000822b6dd
😎 Deploy Preview https://deploy-preview-46--ods-charts.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jacques-lebourgeois jacques-lebourgeois marked this pull request as draft December 15, 2023 11:21
@jacques-lebourgeois jacques-lebourgeois force-pushed the 18-building-angular-test-display-the-warning-on-ods-charts-dependency branch from 78a8d78 to 5c253eb Compare December 15, 2023 16:38
@jacques-lebourgeois jacques-lebourgeois changed the title WIP fix(lib): Generate a module version of the library to be used as import in a devlopment source Dec 15, 2023
@jacques-lebourgeois jacques-lebourgeois marked this pull request as ready for review December 15, 2023 16:48
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

Here are a few first comments for this PR.

@@ -8,7 +8,8 @@
"files": [
"./dist/**/*"
],
"main": "./dist/ods-charts.js",
Copy link
Member

Choose a reason for hiding this comment

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

The main file here should remain dist/ods-charts.js because this would be the default packaged version. The module version shouldn't be the default one.

For example, in Bootstrap:

"main": "dist/js/bootstrap.js",
"module": "dist/js/bootstrap.esm.js",

Copy link
Member Author

Choose a reason for hiding this comment

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

I test this solution (with the changes in webpack.config.js as well of course), and i got back the angular compilation errors :

./src/app/graph/graph.component.ts:44:21-46 - Error: export 'getThemeManager' (imported as 'ODSCharts') was not found in 'ods-charts' (module has no exports)
./src/app/graph/graph.component.ts:45:14-43 - Error: export 'ODSChartsMode' (imported as 'ODSCharts') was not found in 'ods-charts' (module has no exports)
./src/app/graph/graph.component.ts:46:27-79 - Error: export 'ODSChartsCategoricalColorsSet' (imported as 'ODSCharts') was not found in 'ods-charts' (module has no exports)
./src/app/graph/graph.component.ts:47:24-78 - Error: export 'ODSChartsSequentialColorsSet' (imported as 'ODSCharts') was not found in 'ods-charts' (module has no exports)
./src/app/graph/graph.component.ts:48:19-54 - Error: export 'ODSChartsLineStyle' (imported as 'ODSCharts') was not found in 'ods-charts' (module has no exports)

NB : to update the angular projet before testing, I do

npm uninstall ods-charts
npm i ../..

...defaultConfig,
output: {
path: path.resolve(__dirname, './dist'),
filename: 'ods-charts-module.js',
Copy link
Member

Choose a reason for hiding this comment

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

Usually, the naming convention used by libraries is the following:

  • ods-charts.js
  • ods-charts.esm.js
  • ods-charts.umd.js

And for the minified versions:

  • ods-charts.min.js
  • ods-charts.esm.min.js
  • ods-charts.umd.min.js

package.json Show resolved Hide resolved
@jacques-lebourgeois jacques-lebourgeois force-pushed the 18-building-angular-test-display-the-warning-on-ods-charts-dependency branch from 5c253eb to d1459eb Compare December 18, 2023 09:13
Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

There are maybe some package.lock.json to regenerate with the latest changes. Could you please merge the main branch inside this one ? I think once done, we might be almost good to go.

@jacques-lebourgeois jacques-lebourgeois marked this pull request as draft June 7, 2024 07:22
@jacques-lebourgeois jacques-lebourgeois force-pushed the 18-building-angular-test-display-the-warning-on-ods-charts-dependency branch from 4d9ff7f to 4c5dc98 Compare November 22, 2024 17:12
@jacques-lebourgeois
Copy link
Member Author

There are maybe some package.lock.json to regenerate with the latest changes. Could you please merge the main branch inside this one ? I think once done, we might be almost good to go.

I just made a rebase

@jacques-lebourgeois
Copy link
Member Author

Oh ! I think it worked but react test is broken ! :(

@jacques-lebourgeois jacques-lebourgeois force-pushed the 18-building-angular-test-display-the-warning-on-ods-charts-dependency branch from 4c5dc98 to e21dbc8 Compare November 22, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Building angular test display the Warning on 'ods-charts' dependency
3 participants