Skip to content

Conversation

vladfrangu
Copy link
Member

While all the compiled code looks fine and functional, testing would still be needed to ensure it still works correctly

@vladfrangu vladfrangu requested a review from B4nan June 16, 2022 12:40
@B4nan
Copy link
Member

B4nan commented Jun 16, 2022

I guess we should require node 14 and ship major versions of everything

@vladfrangu
Copy link
Member Author

Ouch... Shapeshift is node 16+ only it seems... I thought nodejs 14 also had it. Might have to either experiment with ow or make shapeshift compatible with node 14

@B4nan
Copy link
Member

B4nan commented Jun 16, 2022

Personally i dont mind node 16 either, those are internal packages so as long as we run things on node 16 internally… but i am not sure about that tbh, i would expect older version

@vladfrangu
Copy link
Member Author

I think a huge chunk internally is still on node 14, so yeah.. will have to look into this

@vladfrangu
Copy link
Member Author

vladfrangu commented Jun 17, 2022

Actually, I don't get why the CI is failing on node 14, as it supports ?. and ??...

Misread the stack, its the ??= that crashes it (and what was added in node 15+)

@mnmkng mnmkng requested review from fnesveda and gippy June 19, 2022 11:52
@vladfrangu
Copy link
Member Author

We made shapeshift be nodejs 14+ compatible, and I feel like we could drop the nodejs 12 CI here, especially since it's no longer even supported by node. Thoughts?

@mnmkng
Copy link
Member

mnmkng commented Jun 20, 2022

@fnesveda @gippy Would be great if you guys could chime in. I'm not sure if this won't break anything on the platform :)

@vladfrangu
Copy link
Member Author

If platform depends in any way on the ow errors, it will break something. Otherwise it should just work the same, but I do wonder if it can be tested somehow 👀

@mnmkng
Copy link
Member

mnmkng commented Jun 20, 2022

Platform uses Meteor which uses its own transpilation. Anything can go wrong 😀

@fnesveda
Copy link
Member

fnesveda commented Jun 21, 2022

@vladfrangu @mnmkng So what's actually the goal of this PR? Would be nice to get some short description of what has changed instead of just a review request out of the blue, so we don't have to play detective across 43 changed files.

@mnmkng
Copy link
Member

mnmkng commented Jun 21, 2022

@fnesveda It's a change to use native ESM imports and modules instead of CJS AFAIK.

I added you because I had no idea if Platform even supports ESM.

@vladfrangu
Copy link
Member Author

The PR makes all the modules in here dual-compile to ESM and CJS, to provide native ESM for those that use ESM instead of a light wrapper with gen-esm-wrapper. It shouldn't change anything in terms of code execution, platform will probably still import the CJS file (we can also build browser compatible bundles if it helps at all, to have them globally available via ApifyConstants for example) if it respects the package.json export declarations 😄

Copy link
Member

@fnesveda fnesveda left a comment

Choose a reason for hiding this comment

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

If it still builds CJS versions as well, and they are the default under main in package.json, then it should be importable / requireable on the platform.

We'll just have to change a few imports since all the files are now bundled, and we can't import from them directly, but nothing major.

Could you release this as a major version, just to make sure we don't accidentally update the packages with Renovate bot etc.?

@fnesveda
Copy link
Member

Also CC @jbartadev we'll probably have to update web to Node 14+ because of this (and other reasons).

Errors that don't appear in editor..yay
@B4nan B4nan changed the title feat: native ESM all the packages feat!: native ESM all the packages Jun 21, 2022
@B4nan B4nan changed the title feat!: native ESM all the packages feat!: dual (native) ESM/CJS support all the packages Jun 21, 2022
@B4nan
Copy link
Member

B4nan commented Jun 21, 2022

Let's see how it works... 🤞

@B4nan B4nan changed the title feat!: dual (native) ESM/CJS support all the packages feat: dual (native) ESM/CJS support all the packages Jun 21, 2022
@B4nan B4nan merged commit daf882e into master Jun 21, 2022
@B4nan B4nan deleted the feat/esm branch June 21, 2022 14:14
@B4nan
Copy link
Member

B4nan commented Jun 21, 2022

Looks like it works fine, already added to crawlee, E2E tests are passing including on platform.

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.

4 participants