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

Export node module as esm #2163

Closed
wants to merge 7 commits into from
Closed

Export node module as esm #2163

wants to merge 7 commits into from

Conversation

jpellizzari
Copy link

@jpellizzari jpellizzari commented May 13, 2022

Closes #1916

Makes #1917 irrelevant.

Has better support by consuming tooling to "pass through" assets like svgs

@jpellizzari jpellizzari requested a review from foot May 13, 2022 15:50
@jpellizzari jpellizzari added the area/ui Issues that require front-end work label May 13, 2022
@jpellizzari jpellizzari requested a review from ozamosi May 13, 2022 15:51
@foot
Copy link
Contributor

foot commented May 13, 2022

The top level module field is there to signal to module consumers that its esm. The ./index.js lines up w/ ./dist/index.js once the package.json is copied into the dist folder.

I did not actually test npm start / npm run build with that change. The top level module field might change some local behaviour in parcel, if so we could force it back w/ more options in the default target (outputFormat).

@jpellizzari
Copy link
Author

The top level module field is there to signal to module consumers that its esm. The ./index.js lines up w/ ./dist/index.js once the package.json is copied into the dist folder.

I did not actually test npm start / npm run build with that change. The top level module field might change some local behaviour in parcel, if so we could force it back w/ more options in the default target (outputFormat).

Tested builds and dev on this branch and it works; I wouldn't expect changes to the lib target to cause issues with the main build.

Copy link
Contributor

@foot foot left a comment

Choose a reason for hiding this comment

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

👌

@ozamosi
Copy link
Contributor

ozamosi commented Jun 6, 2022

Is there anything we need to do enterprise-side before we merge this, or can I just merge it?

@jpellizzari
Copy link
Author

I think this needs to be merged first?: weaveworks/weave-gitops-enterprise#755

@jpellizzari
Copy link
Author

Closing as stale, I will take another crack at this once weaveworks/weave-gitops-enterprise#2543 gets merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui Issues that require front-end work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI data-url in-lining makes builds take forever
3 participants