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

chore(dev-mashup): Fix broken yarn start #893

Merged
merged 29 commits into from
Aug 15, 2022

Conversation

johanlahti
Copy link
Contributor

@johanlahti johanlahti commented Aug 15, 2022

Motivation

With parcel v.2 we need to explicitly specify type="module" for <script> tags and cannot specify browser, module, main with an .html extension (parcel will throw an error).

Reference: https://parceljs.org/blog/rc0/#improved-library-builds

Requirements checklist

  • Api specification
    • Ran yarn spec
      • No changes
        OR
      • API changes has been formally approved
  • Unit/Component test coverage
  • Correct PR title for the changes (fix, chore, feat)

When build and tests have passed:

  • Add code reviewers, for example @qlik-oss/nebula-core

Johan Lahti and others added 29 commits January 28, 2021 14:21
@johanlahti johanlahti requested a review from Caele August 15, 2022 13:12
@LiKang6688
Copy link
Contributor

Could you clean your commits a bit, please?

@@ -2,7 +2,7 @@
"name": "local-mashup",
"version": "1.0.0",
"description": "Simple local mashup",
"main": "index.html",
"main": "index.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know why the main is needed? I do not find the main section in https://github.com/qlik-oss/nebula.js/blob/master/commands/create/templates/mashup/_package.json.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for npm or for the compiler/build tool to know which file is the entry point of the application. So no real use for it in this case. We could also remove it completely but it will still default to index.js which is why I decided to keep it (no strong opinion about it though).

Copy link
Contributor

Choose a reason for hiding this comment

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

Very good to know. Then it is fine to keep it.

Copy link
Contributor

@LiKang6688 LiKang6688 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 too many unneeded commits that can be rebased, otherwise looks nice.

@johanlahti johanlahti merged commit 3ee2462 into qlik-oss:master Aug 15, 2022
@johanlahti johanlahti deleted the fix-yarn-start branch August 15, 2022 13:58
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.

None yet

2 participants