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

build: es6 #6720

Closed
wants to merge 1 commit into from
Closed

build: es6 #6720

wants to merge 1 commit into from

Conversation

tim-lai
Copy link
Contributor

@tim-lai tim-lai commented Dec 11, 2020

Description

create valid es modules instead of a es-bundle.

Motivation and Context

How Has This Been Tested?

  • works as 'npm link' to swagger-editor

  • does NOT work with proposed Jest test

Screenshots (if appropriate):

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

@tim-lai
Copy link
Contributor Author

tim-lai commented Dec 11, 2020

"test:artifact:es" is not yet in CI test due to current error below

... and using import { resolve} from... yields same error.

    SyntaxError: Cannot use import statement outside a module

    > 1 | import resolve from "swagger-client/es/resolver"
        | ^
      2 | import { execute, buildRequest } from "swagger-client/es/execute"
      3 | import Http, { makeHttp, serializeRes } from "swagger-client/es/http"
      4 | import resolveSubtree from "swagger-client/es/subtree-resolver"

@tim-lai tim-lai requested a review from char0n December 11, 2020 22:43
@tim-lai
Copy link
Contributor Author

tim-lai commented Dec 12, 2020

update: this change will also necessitate changes to swagger-ui-react build, due to change in module path.

@char0n
Copy link
Member

char0n commented Dec 14, 2020

The PR that changed how we use swagger-client in swagger-ui: #6208

@@ -1 +1,2 @@
dist/
es/
Copy link
Member

Choose a reason for hiding this comment

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

we should also add es/ to .gitignore

@char0n
Copy link
Member

char0n commented Dec 14, 2020

My conclusion after analysis:

swagger-client issues

swagger-client issues will be resolved by adding transformIgnorePatterns: ['/node_modules/(?!swagger-client)', '\\.pnp\\.[^\\\/]+$'] by adding into jest config.

But this is not a solution to a problem. When swagger-client is out of the way, you will intercept many other similar errors. The reason is that es/ version of swagger-ui uses ES6 import to import other resources then JavaScript files; namely YAML files, images, styles etc... So using the same system for testing ES build fragments as used in swagger-client will not work here. Testing es/ build artifacts in swagger-client works because there are no imports to files except JavaScript files. In swagger-ui, completely different strategy needs to be utilized.

Proposed strategy of ES build fragment testing

In order to propertly test es/ build fragment, we need to bundle it to JavaScript bundle and then test the result of this bundling. Before the tests start, we'll use webpack to create UMD bundle from es/ build fragment (setup up phase). Then we test the UMD bundle it's default export is a function named SwaggerUI. After the test, we delete the UMD bundle (tear down). More proper test though would be an e2e test of above mentioned UMD bundle, verifying that it displays SwaggerUI.

@DreierF
Copy link
Contributor

DreierF commented Jan 17, 2021

I'm pretty sure these changes alone won't produce a valid ESM bundle that can be consumed by other non-Webpack bundlers. The reason is that besides the already mentioned yaml and image imports swagger-ui also uses Webpack's require.context API which needs to be resolved at build time to be consumable by other bundlers.
See for more details: #6415 (comment)

@char0n
Copy link
Member

char0n commented Jan 17, 2021

@DreierF thanks for the comment.

You're right require.context is something that even e2e wouldn't catch because we'd use webpack to build the es/ fragments. I see two options, either get rid of require.context in code or use webpack to create one big es-bundle.js file which internally uses ES6 import statements to import vendor code from node_modules (tree-shakeable bundled code with ES6 imports).

@gsdatta
Copy link

gsdatta commented Oct 8, 2021

Hey folks, running into large bundle sizes for similar reasons. Anything I can do to help with getting this PR merged in?

@davidroman0O
Copy link

davidroman0O commented Mar 24, 2022

Just here to confirm this issue is the same when with next build
If you're importing directly import X from "swagger-react-ui" if won't directly display the error message but using the dynamic api of next will give:

SyntaxError: Cannot use import statement outside a module
    at Object.compileFunction (node:vm:352:18)
    at wrapSafe (node:internal/modules/cjs/loader:1026:15)
    at Module._compile (node:internal/modules/cjs/loader:1061:27)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1149:10)
    at Module.load (node:internal/modules/cjs/loader:975:[32](https://gitlab-ncsa.ubisoft.org/maas/web/-/jobs/44414865#L32))
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Module.require (node:internal/modules/cjs/loader:999:19)
    at require (node:internal/modules/cjs/helpers:102:18)
    at Object.<anonymous> (/node_modules/swagger-ui-react/commonjs.js:10:53)
    at Module._compile (node:internal/modules/cjs/loader:1097:14)

Just like the post of @tim-lai

@discapes
Copy link

discapes commented Sep 6, 2022

@davidroman0O did u find a solution?

@char0n
Copy link
Member

char0n commented Jun 12, 2023

This has been solved in other PRs. Currently we have following package.json mapping:

  "main": "./dist/swagger-ui.js",
  "module": "./dist/swagger-ui-es-bundle-core.js",
  "exports": {
    "./dist/swagger-ui.css": "./dist/swagger-ui.css",
    "./dist/oauth2-redirect.html": "./dist/oauth2-redirect.html",
    "./dist/swagger-ui-standalone-preset": "./dist/swagger-ui-standalone-preset.js",
    ".": {
      "import": "./dist/swagger-ui-es-bundle-core.js",
      "require": "./dist/swagger-ui.js"
    }
  },

./dist/swagger-ui-es-bundle-core.js - is a pure ESM bundle.

@char0n char0n closed this Jun 12, 2023
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.

6 participants