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

Create-Plugin: Transform known ES modules in jest config #145

Merged
merged 3 commits into from
Nov 17, 2022

Conversation

jackw
Copy link
Collaborator

@jackw jackw commented Nov 14, 2022

What this PR does / why we need it:
The previous PR that addressed issues with loading es modules by transforming all modules in node_modules introduced an issue where SWC will error if a package contains code the typescript compiler doesn't appreciate.

One example of this is reassignment of a class (something we do in grafana/data).

I'm not sure if there's a better way to solve this but I'm now leaning towards maintaining a list of packages as a solution mentioned in #111.

Which issue(s) this PR fixes:

Fixes #111

Special notes for your reviewer:

📦 Published PR as canary version: Canary Versions

✨ Test out this PR locally via:

npm install @grafana/create-plugin@0.5.2-canary.145.6b7c318.0
# or 
yarn add @grafana/create-plugin@0.5.2-canary.145.6b7c318.0

@jackw jackw added bug create-plugin related to the create-plugin tool patch Increment the patch version when merged labels Nov 14, 2022
@jackw jackw added this to the v1.0.0 milestone Nov 14, 2022
@jackw jackw self-assigned this Nov 14, 2022
@jackw jackw added the release Create a release when this pr is merged label Nov 14, 2022
leventebalogh
leventebalogh previously approved these changes Nov 14, 2022
Copy link
Collaborator

@leventebalogh leventebalogh left a comment

Choose a reason for hiding this comment

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

Nice! 👏

@@ -5,6 +5,8 @@
*/

const path = require('path');
// Jest will throw errors if it tries to load an es module with it being transformed first
const esModules = ['ol', 'react-colorful'].join('|');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it would make sense to update the comment and the name of the variable to reflect more what problems one can solve with adding package names here and when it should be used? Probably it would also make sense to export it so plugin devs can extend it in their own config more easily?

Suggested change
const esModules = ['ol', 'react-colorful'].join('|');
// Just an idea, maybe we can find a better name
const esModuleNpmPackages = ['ol', 'react-colorful'].join('|');

@jackw jackw dismissed leventebalogh’s stale review November 17, 2022 07:09

Significant code changes

Copy link
Collaborator

@leventebalogh leventebalogh left a comment

Choose a reason for hiding this comment

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

🚀

@jackw jackw changed the title Create-Plugin: Ignore known ES modules in jest config Create-Plugin: Transform known ES modules in jest config Nov 17, 2022
@jackw jackw merged commit 51917d4 into main Nov 17, 2022
@jackw jackw deleted the jackw/jest-esm-packages branch November 17, 2022 14:30
@grafanabot
Copy link
Contributor

🚀 PR was released in @grafana/create-plugin@0.5.3 🚀

@grafanabot grafanabot added the released This issue/pull request has been released. label Nov 17, 2022
@tolzhabayev
Copy link
Collaborator

tolzhabayev commented Nov 17, 2022

@jackw This causes some issues, maybe something missing in the migration script?

Error: Cannot find module './jest/utils'
Require stack:
- /drone/src/.config/jest.config.js

https://drone.grafana.net/grafana/clock-panel/35/1/5
grafana/clock-panel#110

@jackw
Copy link
Collaborator Author

jackw commented Nov 18, 2022

@jackw This causes some issues, maybe something missing in the migration script?


Error: Cannot find module './jest/utils'

Require stack:

- /drone/src/.config/jest.config.js

https://drone.grafana.net/grafana/clock-panel/35/1/5

grafana/clock-panel#110

Thanks for the heads up @tolzhabayev I'll get it fixed! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
create-plugin related to the create-plugin tool patch Increment the patch version when merged release Create a release when this pr is merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jest tests are failing due to some node_modules not being transpiled before loading
4 participants