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

Parser does not work out of the box in non-node environments (websites) #344

Closed
jonaslagoni opened this issue Aug 19, 2021 · 20 comments · Fixed by #572
Closed

Parser does not work out of the box in non-node environments (websites) #344

jonaslagoni opened this issue Aug 19, 2021 · 20 comments · Fixed by #572
Labels

Comments

@jonaslagoni
Copy link
Member

Describe the bug

If the parser is utilized in a website the path parameters is a requirement to set, in order to make the library work.

How to Reproduce

Given the AsyncAPI document:

{
    "asyncapi": "2.1.0",
    "info": {
        "title": "Streetlights API",
        "version": "1.0.0",
        "description": "The Smartylighting Streetlights API allows you\nto remotely manage the city lights.\n",
        "license": {
            "name": "Apache 2.0",
            "url": "https://www.apache.org/licenses/LICENSE-2.0"
        }
    },
    "servers": {
        "mosquitto": {
            "url": "mqtt://test.mosquitto.org",
            "protocol": "mqtt"
        }
    },
    "channels": {
        "light/measured": {
            "publish": {
                "summary": "Inform about environmental lighting conditions for a particular streetlight.",
                "operationId": "onLightMeasured",
                "message": {
                    "name": "LightMeasured",
                    "payload": {
                        "type": "object",
                        "$id": "LightMeasured",
                        "properties": {
                            "id": {
                                "type": "integer",
                                "minimum": 0,
                                "description": "Id of the streetlight."
                            },
                            "lumens": {
                                "type": "integer",
                                "minimum": 0,
                                "description": "Light intensity measured in lumens."
                            },
                            "sentAt": {
                                "type": "string",
                                "format": "date-time",
                                "description": "Date and time when the message was sent."
                            }
                        }
                    }
                }
            }
        }
    }
}

and the following code:

const parsedInput = await parse(input)

which forces the path parameter:

const parsedInput = await parse(input, {path: './'})

This is because the following code:

options.path = options.path || `${process.cwd()}${path.sep}`;

Where in a browser environment process.cwd() returns / and path.sep returns / which together (//) is an incorrect path and it will throw an exception once it begins the dereference process.

Expected behavior

Expected the parser to work out of the box without any modification and ensure the default path is always valid.

@jonaslagoni jonaslagoni added the bug Something isn't working label Aug 19, 2021
@fmvilas fmvilas added the good first issue Good for newcomers label Aug 19, 2021
@derberg
Copy link
Member

derberg commented Aug 23, 2021

you mean the generated bundle doesn't work or?

@jonaslagoni
Copy link
Member Author

you mean the generated bundle doesn't work or?

@derberg when the parser is a secondary dependency, i.e. when I import Modelina in the website, it is using the default package. This simply cannot be used as-is since the dereference logic throws an error, because the path // is not a valid one.

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Aug 23, 2021

@jonaslagoni Did you try import the bundle from @asyncapi/parser/dist/bundle.js path?

@derberg
Copy link
Member

derberg commented Aug 23, 2021

@jonaslagoni don't tell me you did not read the docs and spent time on reporting an issue 😛

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Aug 23, 2021

@derberg @magicmatatjahu so you want me to import @asyncapi/parser/dist/bundle.js here https://github.com/asyncapi/modelina/blob/8d080319ce9b9ba41d9a5dbaa182306626f498b0/src/processors/AsyncAPIInputProcessor.ts#L1 ?

But only when used in browsers? 🤨 That does not make sense to me? Especially since the only thing not working in the parser is the dereference logic...

@jonaslagoni don't tell me you did not read the docs and spent time on reporting an issue 😛

It is not me that is importing the parser, Modelina is, I am just using it (Modelina). But yes, I read it, I just don't think it applies here 😄

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Aug 23, 2021

Ok I see... I was face with similar problem one time when I tried run the react-component in the NextJS app and fix the issue - asyncapi/asyncapi-react#197 and as I remember the NextJS under the hood has custom webpack which will transform the code to the optimized bundle, but problem is how it it handles the node-related packages like path or Buffer class. Webpack 4 "tries" to resolve problems but for Webpack5 you must define all related node-packages the corresponding browser-compatible polyfills (really... people hate this approach introduces in the v5 version).

But about NextJS... If you have enables the SSG (static site generator) then NextJS tries to use the browser-compatible packages of your imports, so it will try to bundle (by default by Webpack4) the parser to the compatible browser version and replace the process, path with the polyfills, but sometimes it creates the issues like this -> asyncapi/asyncapi-react#197 and in the react-component repo we bundle the component to the separate browser version and then you can use it in the browser without problems, even in NextJS. I don't know how, but seems that NextJS has "custom" webpack that miss bundle something correctly.

I thought that only react-component will have such an issues related to the parser, but as I see also the modelina have such an issues... Maybe we should start investigation how to write two corresponding Parser classes, one for node and one for browser? This second shouldn't use the node-related dependencies (of course with full support of tree-shakable). What do you think?

@jonaslagoni
Copy link
Member Author

jonaslagoni commented Aug 23, 2021

Maybe we should start investigation how to write two corresponding Parser classes, one for node and one for browser? This second shouldn't use the node-related dependencies (of course with full support of tree-shakable). What do you think?

@magicmatatjahu I think that makes sense, but in many cases, I dont think that the node-related dependencies matter (any more or at least in some environments), as for example in the website when I used Modelina, all it complained about was that it could not dereference //, which is fixed with the option { path: './'}. After that everything worked fine, of course, local references won't work at all, but yea, it can't in that environment anyway. Of course, I am not using the "read file" functionality of the parser, not sure if it works 🤔

You can see the "workaround" here: https://github.com/asyncapi/website/blob/3b18f671784670cb9740e3d262ed4c0c57139d58/pages/modelina.js#L86. I am however relying on the transitive dependency from Modelina (for the parser).

So in most cases, the parser could be used as-is in many cases, and in special website environments it would need the non-node-related dependency version 🤔 Or am I missing something?

@magicmatatjahu
Copy link
Member

So in most cases, the parser could be used as-is in many cases, and in special website environments it would need the non-node-related dependency version

Exactly, in some environments it can be handy to have node-free Parser. However, I am glad that workaround works, but tbh... 😅 We don't use such an input (I mean option to the parse function) and nobody complain it as problem 🤔

@jonaslagoni
Copy link
Member Author

Exactly, in some environments it can be handy to have node-free Parser. However, I am glad that workaround works, but tbh... 😅 We don't use such an input (I mean option to the parse function) and nobody complain it as problem 🤔

@magicmatatjahu it is because of this (when I don't use the bundled package version):

Where in a browser environment process.cwd() returns / and path.sep returns / which together (//) is an incorrect path and it will throw an exception once it begins the dereference process.

I doubt many use it in this combination between node and website versions, as many probably switch to the package version @asyncapi/parser/dist/bundle.js and all is good, I suppose 🤷

@magicmatatjahu
Copy link
Member

I am curious because we also use the normal package in the react-component - https://github.com/asyncapi/asyncapi-react/blob/next/library/src/helpers/parser.ts#L1 and everything works.

@jonaslagoni
Copy link
Member Author

🧐 That is weird yea...

Gonna remove good-first-issue for now until we have a clear direction we want to take.

@jonaslagoni jonaslagoni removed the good first issue Good for newcomers label Aug 23, 2021
@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Dec 22, 2021
@derberg
Copy link
Member

derberg commented Dec 22, 2021

thb I still don't really understand what is the issue and why bundle.js is not good enough

@derberg derberg removed the stale label Dec 22, 2021
@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Apr 22, 2022
@derberg
Copy link
Member

derberg commented Apr 26, 2022

@jonaslagoni any updates?

@github-actions github-actions bot removed the stale label Apr 27, 2022
@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added stale and removed stale labels Aug 25, 2022
@jonaslagoni
Copy link
Member Author

I have not looked at this since.

thb I still don't really understand what is the issue and why bundle.js is not good enough

The "issue" is that

options.path = options.path || `${process.cwd()}${path.sep}`;
is built for node environments, and fails when used in a website environment as explained.

I think the way forward is one of these:

  1. Better explain in the docs that you have to manually set the path argument if not used within a node environment and why
  2. Better handle the default path argument so it always produces a correct path that can be used by default, I.e. that we somehow know which environment the parser is being used in.

@derberg
Copy link
Member

derberg commented Sep 13, 2022

shouldn't https://github.com/asyncapi/parser-js/pull/572/files fix the issue?

@asyncapi-bot
Copy link
Contributor

🎉 This issue has been resolved in version 1.16.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@asyncapi-bot
Copy link
Contributor

🎉 This issue has been resolved in version 2.0.0-next-major.18 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants