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

Allow import of non-js files via exports in package.json #10711

Merged
merged 2 commits into from
Aug 31, 2022

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented Aug 22, 2022

Fixes #9212

This allows users to import non-JS files from the CesiumJS package, including CSS files and images, using subpath patterns, available in NodeJS 12 and above. I explicitly disallowed absolute path imports for JS files as it eliminates ambiguity about importing modules from the default ("cesium"), and avoids a situation where typescript looks for co-located definition files instead of at the main types location.

I allow both Source and Build exports as it allows users some flexibility based on their build tooling: Some may want to use the pre-bundled and minified CSS from the build folder (as recommended in our Quickstart Guide), while others may prefer handling the bundling and magnification separately.

This can be tested using a bundler such as Rollup or Webpack, eg. https://github.com/CesiumGS/cesium-webpack-example.

@cesium-concierge
Copy link

Thanks for the pull request @ggetz!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

@sanjeetsuhag
Copy link
Contributor

@ggetz Could you provide an example of an import for a CSS file, for example, using this subpath pattern? I'm trying it in cesium-webpack-example and instead of import "cesium/Widgets/widgets.css";, I am trying to import the file from Build.

@ggetz
Copy link
Contributor Author

ggetz commented Aug 29, 2022

@sanjeetsuhag From the built version, the import would be import "cesium/Build/Cesium/Widgets/widgets.css";

@sanjeetsuhag
Copy link
Contributor

I ran the following commands in the cesium repo with this branch:

npm install
npm run release # To generate the Build/Cesium folder
npm link

In cesium-webpack-example, I changed the CSS import to import "cesium/Build/Cesium/Widgets/widgets.css"; and ran:

npm install
npm link cesium
npm start

I get the following error:

Compiled with problems:

ERROR in ./src/index.js 2:0-49

Module not found: Error: Can't resolve 'cesium/Build/Cesium/Widgets/widgets.css' in 'C:\Users\suhag\Developer\cesium-webpack-example\src'

@ggetz
Copy link
Contributor Author

ggetz commented Aug 31, 2022

@sanjeetsuhag

In webpack.config.js, the following change is also needed:

    resolve: {
-       alias: {
-           cesium: path.resolve(__dirname, cesiumSource)
-       },
+       fallback: { "https": false, "zlib": false, "http": false },
        mainFiles: ['index', 'Cesium']
    },

The alias field should no longer be required, even before this change. The fallback field is now specified because of #10694. The recommended webpack config is really due for an update and overall simplification.

@sanjeetsuhag
Copy link
Contributor

@ggetz Alright, that cleared it up. Everything works as expected! Just need to merge in main.

@ggetz
Copy link
Contributor Author

ggetz commented Aug 31, 2022

Thanks @sanjeetsuhag, should be ready to go!

Copy link
Contributor

@sanjeetsuhag sanjeetsuhag left a comment

Choose a reason for hiding this comment

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

Thanks @ggetz!

@sanjeetsuhag sanjeetsuhag merged commit 4493e84 into main Aug 31, 2022
@sanjeetsuhag sanjeetsuhag deleted the export-non-js branch August 31, 2022 16:59
@thw0rted
Copy link
Contributor

Did you test the part that's supposed to disallow js imports (with null value)? The Node docs say the entries there are evaluated in order, so if a js path hits the general wildcard first, I think it would use that and never reach the null.

@ggetz
Copy link
Contributor Author

ggetz commented Sep 1, 2022

Yes, I was not able to import JS files with this configuration. For example, importing from cesium/Source/Core/Cartesian3.js directly will throw an error. From the docs, the evaluation order appears to apply to conditional exports, but I don't see a similar caveat for subpath patterns. The subpath pattern example is ordered general to specific, as we have here.

@thw0rted
Copy link
Contributor

thw0rted commented Sep 1, 2022

Yeah that's pretty confusing, I may double check on the Node tracker if they want to specify an order of operations / precedence -- the docs definitely don't make it explict. Thanks for tracking it down!

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.

Can not import widgets.css file not exported from package.json
4 participants