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

use ESM Modules for dev scripts #701

Closed
9 tasks
MarcusNotheis opened this issue Oct 5, 2020 · 8 comments · Fixed by #720
Closed
9 tasks

use ESM Modules for dev scripts #701

MarcusNotheis opened this issue Oct 5, 2020 · 8 comments · Fixed by #720
Assignees

Comments

@MarcusNotheis
Copy link
Contributor

MarcusNotheis commented Oct 5, 2020

Since we are using Node 12.16 or 14 for developing and building UI5 Web Components for React, we can now migrate all internal dev scripts and helper files to native esm modules.

Todo

We can probably add "type": "module" to the top-level package.json, renaming config files (such as .eslintrc.js) to .cjs.

  • Mention the required Node-Version in docs/Guidelines.md (^12.16 || ^14)

Migrate the following files to use ESM Modules:

  • .storybook/main.js
  • config/jest.config.js
  • config/paths.js
  • everything under scripts/*
  • shared/tests/serializer/*
  • babel.config.js
  • packages/base/scripts/cssVariables/parse.js
  • packages/main/scripts/create-library-export.js

Useful Links

@KolCrooks
Copy link
Contributor

I can do this if you want!

@MarcusNotheis
Copy link
Contributor Author

Sure, I've just assigned this issue to you. Thanks for tackling this one 👍

@KolCrooks
Copy link
Contributor

I changed all the files to use ESM Modules but for some reason it has had some unforeseen side effects. When It try and build the code, I get this error.
image
It looks like changing the module type has somehow added a second package/ in the directory URL. I think this is associated with a problem that I found when running tests. In config/paths.js it uses __dirname but when I run it with the tests, it says that __dirname is not defined and errors out.
image

You can look at the changes I made here: KolCrooks@b2567f3.
I'm not sure if you want to continue with theses changes to the code because I really don't how to proceed with all of this.

@MarcusNotheis
Copy link
Contributor Author

MarcusNotheis commented Oct 6, 2020

Hey @KolCrooks,
thanks for starting here. I think we need some more adjustments but we are on a good way:

  • Rename all rollup.config.js to rollup.config.mjs (we need that because the package.json files of the single modules do not contain type: module
  • when importing relative paths like rollupConfigFactory, utils, or PATHS, make sure you add .js to the import, like: import rollupConfigFactory from '../../scripts/rollup/configFactory.js';
  • For the missing __dirname, this might help you
  • scripts/utils is importing import { ncp } from 'ncp';, for some reason this needs to change to import ncp from 'ncp';

I was playing around with the mentioned changes above and it looked promising. I didn't get it running with node 12, but node 14 was working fine.

@KolCrooks
Copy link
Contributor

I think that I created something that builds. I am getting some problems with the yarn run build:types stage of the build process but I think that it is just associated with my system environment. On that note, the command "build:polyfills": "tsc ./src/polyfill/*.ts --outDir ./polyfill --skipLibCheck" in the base package will only run on linux. I tried it on windows but got errors about not being able to find the file ./src/polyfill/*.ts so I ended up just running it on WSL.
You can check out the changes here: https://github.com/KolCrooks/ui5-webcomponents-react/tree/ESMDevScripts

@MarcusNotheis
Copy link
Contributor Author

Thanks @KolCrooks,
the build is looking good! Do you want to open a Pull Request so that we can get this over the line?
There are two things I noticed:

  • You can change the Babel import in the config factory file:

    REPLACE THIS
    import Babel from '@rollup/plugin-babel';
    WITH THIS
    import { babel } from '@rollup/plugin-babel';

    and remove const { babel } = Babel; from line 14.

  • Please remove the package-lock.json file as we are only using yarn.

There is one more thing regarding the storybook: running yarn start is currently throwing an error on my side, but I think this might be related to storybookjs/storybook#11587.

@MarcusNotheis
Copy link
Contributor Author

I think I found a workaround for the Storybook Issue.

  1. Discard all changes in the .storybook/main.js file and rename it to .storybook/main.cjs
  2. replace const PATHS = require('../config/paths'); with const root = path.resolve(__dirname, '..');
  3. replace all occurrences of PATHS.root with root

@KolCrooks
Copy link
Contributor

@MarcusNotheis Sorry that I am just responding now but I have been really busy recently.

When I had

import Babel from '@rollup/plugin-babel';
AS
import { babel } from '@rollup/plugin-babel';

It would give an error telling me that I could only use the default import.
image
I also made a pull request to merge the changes into the main branch. #720

@MarcusNotheis MarcusNotheis linked a pull request Oct 9, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants